diff --git a/controllers/component_controller_test.go b/controllers/component_controller_test.go index 261b51d37..5c83d1077 100644 --- a/controllers/component_controller_test.go +++ b/controllers/component_controller_test.go @@ -1494,7 +1494,7 @@ var _ = Describe("Component controller", func() { Expect(createdHasComp.Spec.Replicas).Should(BeNil()) //Update replica - updatedHasComp.Spec.Replicas = &numReplica + updatedHasComp.Spec.Replicas = &oneReplica Expect(k8sClient.Update(ctx, updatedHasComp)).Should(Succeed()) newUpdatedHasComp := &appstudiov1alpha1.Component{} Eventually(func() bool { @@ -1505,7 +1505,7 @@ var _ = Describe("Component controller", func() { Expect(newUpdatedHasComp.Status.Conditions[len(newUpdatedHasComp.Status.Conditions)-1].Status).Should(Equal(metav1.ConditionTrue)) //replica should not be nil and should have a value Expect(newUpdatedHasComp.Spec.Replicas).Should(Not(BeNil())) - Expect(*newUpdatedHasComp.Spec.Replicas).Should(Equal(numReplica)) + Expect(*newUpdatedHasComp.Spec.Replicas).Should(Equal(oneReplica)) // Delete the specified HASComp resource deleteHASCompCR(hasCompLookupKey) diff --git a/controllers/update.go b/controllers/update.go index ed2338bcb..9e95a21b1 100644 --- a/controllers/update.go +++ b/controllers/update.go @@ -70,24 +70,84 @@ func (r *ComponentReconciler) updateComponentDevfileModel(req ctrl.Request, hasC for _, kubernetesComponent := range kubernetesComponents { compUpdateRequired := false // Update for Replica - currentReplica := 0 + currentReplica := 1 // default value is 1 + keyFound := true + if len(kubernetesComponent.Attributes) == 0 { kubernetesComponent.Attributes = attributes.Attributes{} + keyFound = false } else { var err error currentReplica = int(kubernetesComponent.Attributes.GetNumber(devfile.ReplicaKey, &err)) if err != nil { if _, ok := err.(*attributes.KeyNotFoundError); !ok { return err + } else { + keyFound = false + currentReplica = 1 //if an error is raised, it'll set currentReplica to 0 so we need to reset back to the default } } } - numReplicas := util.GetIntValue(component.Spec.Replicas) - if currentReplica != numReplicas { - log.Info(fmt.Sprintf("setting devfile component %s attribute component.Spec.Replicas to %v", kubernetesComponent.Name, numReplicas)) - kubernetesComponent.Attributes = kubernetesComponent.Attributes.PutInteger(devfile.ReplicaKey, numReplicas) - compUpdateRequired = true + numReplicas := 1 //default value + if component.Spec.Replicas != nil { + numReplicas = util.GetIntValue(component.Spec.Replicas) + // Component.Spec.Replicas will override any other settings. + // We will write the attribute if it doesn't exist for the initial creation case when comp.spec.replica is 1 + if currentReplica != numReplicas || !keyFound { + log.Info(fmt.Sprintf("setting devfile component %s attribute %s to %v", kubernetesComponent.Name, devfile.ReplicaKey, numReplicas)) + kubernetesComponent.Attributes = kubernetesComponent.Attributes.PutInteger(devfile.ReplicaKey, numReplicas) + compUpdateRequired = true + } + } else { + //check to see if we have an inlined deployment + isDeployReplicaSet := false + inlined := kubernetesComponent.Kubernetes.Inlined + if inlined != "" { + log.Info(fmt.Sprintf("reading the kubernetes inline from component %s", component.Name)) + src := parser.YamlSrc{ + Data: []byte(inlined), + } + + values, err := parser.ReadKubernetesYaml(src, nil, nil) + if err != nil { + return err + } + + resources, err := parser.ParseKubernetesYaml(values) + if err != nil { + return err + } + + if len(resources.Deployments) > 0 { + replica := resources.Deployments[0].Spec.Replicas + if replica != nil { + isDeployReplicaSet = true + //remove the deployment/replicas attribute which can be left behind if we go from a set value to an unset value + if kubernetesComponent.Attributes.Exists(devfile.ReplicaKey) { + var err error + num := int(kubernetesComponent.Attributes.GetNumber(devfile.ReplicaKey, &err)) + + if err != nil { + if _, ok := err.(*attributes.KeyNotFoundError); !ok { + return err + } else { + log.Info(fmt.Sprintf("deleting %s attribute with value %v", devfile.ReplicaKey, num)) + delete(kubernetesComponent.Attributes, devfile.ReplicaKey) + } + } + + } + } + } + } + + //set the default if replicas is unset in the component and deployment spec. + if !isDeployReplicaSet { + log.Info(fmt.Sprintf("setting devfile component %s attribute component.Spec.Replicas to %v", kubernetesComponent.Name, 1)) + kubernetesComponent.Attributes = kubernetesComponent.Attributes.PutInteger(devfile.ReplicaKey, 1) + compUpdateRequired = true + } } // Update for Port diff --git a/controllers/update_test.go b/controllers/update_test.go index c9bb25094..f29beb01a 100644 --- a/controllers/update_test.go +++ b/controllers/update_test.go @@ -44,7 +44,60 @@ import ( "sigs.k8s.io/yaml" ) -var numReplica = 1 +var ( + oneReplica = 1 + zeroReplica = 0 + threeReplicas = 3 +) + +var k8sInlined = ` + apiVersion: apps/v1 + kind: Deployment + metadata: + creationTimestamp: null + labels: + maysun: test + name: deploy-sample + spec: + replicas: 3 + selector: {} + strategy: {} + template: + metadata: + creationTimestamp: null + labels: + app.kubernetes.io/instance: component-sample + spec: + containers: + - env: + - name: FOO + value: foo1 + - name: BARBAR + value: bar1 + image: quay.io/redhat-appstudio/user-workload:application-service-system-component-sample + imagePullPolicy: Always + livenessProbe: + httpGet: + path: / + port: 1111 + initialDelaySeconds: 10 + periodSeconds: 10 + name: container-image + ports: + - containerPort: 1111 + readinessProbe: + initialDelaySeconds: 10 + periodSeconds: 10 + tcpSocket: + port: 1111 + resources: + limits: + cpu: "2" + memory: 500Mi + requests: + cpu: 700m + memory: 400Mi + status: {}` func TestUpdateApplicationDevfileModel(t *testing.T) { tests := []struct { @@ -617,6 +670,7 @@ func TestUpdateComponentDevfileModel(t *testing.T) { } envAttributes := attributes.Attributes{}.FromMap(map[string]interface{}{devfilePkg.ContainerENVKey: []corev1.EnvVar{{Name: "FOO", Value: "foo"}}}, &err) + emptyAttributes := attributes.Attributes{} if err != nil { t.Error(err) } @@ -638,6 +692,7 @@ func TestUpdateComponentDevfileModel(t *testing.T) { component appstudiov1alpha1.Component updateExpected bool wantErr bool + wantReplica *int }{ { name: "No kubernetes component", @@ -678,13 +733,248 @@ func TestUpdateComponentDevfileModel(t *testing.T) { }, }, Route: "route1", - Replicas: &numReplica, + Replicas: &oneReplica, TargetPort: 1111, Env: env, Resources: originalResources, }, }, updateExpected: true, + wantReplica: &oneReplica, + }, + { + // This is the default test case where replicas is unset everywhere + name: "replica test: no attributes, comp.Spec.Replicas is nil, and no deployment spec. Replica should be 1", + components: []devfileAPIV1.Component{ + { + Name: "component1", + ComponentUnion: devfileAPIV1.ComponentUnion{ + Kubernetes: &devfileAPIV1.KubernetesComponent{}, + }, + }, + }, + component: appstudiov1alpha1.Component{ + Spec: appstudiov1alpha1.ComponentSpec{ + ComponentName: "componentName", + Application: "applicationName", + Source: appstudiov1alpha1.ComponentSource{ + ComponentSourceUnion: appstudiov1alpha1.ComponentSourceUnion{ + GitSource: &appstudiov1alpha1.GitSource{ + URL: "url", + }, + }, + }, + Route: "route1", + TargetPort: 1111, + Env: env, + Resources: originalResources, + }, + }, + updateExpected: true, + wantReplica: &oneReplica, + }, + { + name: "replica test: comp.Spec.Replicas is 3, no deployment spec. Replicas should be 3", + components: []devfileAPIV1.Component{ + { + Name: "component1", + ComponentUnion: devfileAPIV1.ComponentUnion{ + Kubernetes: &devfileAPIV1.KubernetesComponent{}, + }, + }, + }, + component: appstudiov1alpha1.Component{ + Spec: appstudiov1alpha1.ComponentSpec{ + ComponentName: "componentName", + Application: "applicationName", + Replicas: &threeReplicas, + Source: appstudiov1alpha1.ComponentSource{ + ComponentSourceUnion: appstudiov1alpha1.ComponentSourceUnion{ + GitSource: &appstudiov1alpha1.GitSource{ + URL: "url", + }, + }, + }, + Route: "route1", + TargetPort: 1111, + Env: env, + Resources: originalResources, + }, + }, + updateExpected: true, + wantReplica: &threeReplicas, + }, + { + // Initial creation case, where the attribute does not exist so an update is expected + name: "replica test: comp.Spec.Replicas is 1, no deployment spec. Replicas should be 1", + components: []devfileAPIV1.Component{ + { + Name: "component1", + ComponentUnion: devfileAPIV1.ComponentUnion{ + Kubernetes: &devfileAPIV1.KubernetesComponent{}, + }, + }, + }, + component: appstudiov1alpha1.Component{ + Spec: appstudiov1alpha1.ComponentSpec{ + ComponentName: "componentName", + Application: "applicationName", + Replicas: &oneReplica, + Source: appstudiov1alpha1.ComponentSource{ + ComponentSourceUnion: appstudiov1alpha1.ComponentSourceUnion{ + GitSource: &appstudiov1alpha1.GitSource{ + URL: "url", + }, + }, + }, + Route: "route1", + TargetPort: 1111, + Env: env, + Resources: originalResources, + }, + }, + updateExpected: true, + wantReplica: &oneReplica, + }, + { + // similar to the initial creation case above, this time attribute list is present but key not found + name: "replica test: comp.Spec.Replicas is 1, no deployment spec. Replicas should be 1", + components: []devfileAPIV1.Component{ + { + Name: "component1", + Attributes: emptyAttributes.PutInteger(devfilePkg.ContainerImagePortKey, 1001), + ComponentUnion: devfileAPIV1.ComponentUnion{ + Kubernetes: &devfileAPIV1.KubernetesComponent{}, + }, + }, + }, + component: appstudiov1alpha1.Component{ + Spec: appstudiov1alpha1.ComponentSpec{ + ComponentName: "componentName", + Application: "applicationName", + + Replicas: &oneReplica, + Source: appstudiov1alpha1.ComponentSource{ + ComponentSourceUnion: appstudiov1alpha1.ComponentSourceUnion{ + GitSource: &appstudiov1alpha1.GitSource{ + URL: "url", + }, + }, + }, + Route: "route1", + TargetPort: 1111, + Env: env, + Resources: originalResources, + }, + }, + updateExpected: true, + wantReplica: &oneReplica, + }, + { + // component.Spec.Replicas is explicitly set as 0, so it should override the deployment replica + name: "replica test: comp.Spec.Replicas is 0, Deployment spec is 3. Replicas should be 0", + components: []devfileAPIV1.Component{ + { + Name: "component1", + ComponentUnion: devfileAPIV1.ComponentUnion{ + Kubernetes: &devfileAPIV1.KubernetesComponent{ + K8sLikeComponent: devfileAPIV1.K8sLikeComponent{ + K8sLikeComponentLocation: devfileAPIV1.K8sLikeComponentLocation{ + Inlined: k8sInlined, + }, + }, + }, + }, + }, + }, + component: appstudiov1alpha1.Component{ + Spec: appstudiov1alpha1.ComponentSpec{ + ComponentName: "componentName", + Application: "applicationName", + Replicas: &zeroReplica, + Source: appstudiov1alpha1.ComponentSource{ + ComponentSourceUnion: appstudiov1alpha1.ComponentSourceUnion{ + GitSource: &appstudiov1alpha1.GitSource{ + URL: "url", + }, + }, + }, + Route: "route1", + TargetPort: 1111, + Env: env, + Resources: originalResources, + }, + }, + updateExpected: true, + wantReplica: &zeroReplica, + }, + { + // No update to the component model because deployment replica will be used + name: "replica test: comp.Spec.Replicas is nil, deployment spec replica is 3. No update", + components: []devfileAPIV1.Component{ + { + Name: "component1", + ComponentUnion: devfileAPIV1.ComponentUnion{ + Kubernetes: &devfileAPIV1.KubernetesComponent{ + K8sLikeComponent: devfileAPIV1.K8sLikeComponent{ + K8sLikeComponentLocation: devfileAPIV1.K8sLikeComponentLocation{ + Inlined: k8sInlined, + }, + }, + }, + }, + }, + }, + component: appstudiov1alpha1.Component{ + Spec: appstudiov1alpha1.ComponentSpec{ + ComponentName: "componentName", + Application: "applicationName", + Source: appstudiov1alpha1.ComponentSource{ + ComponentSourceUnion: appstudiov1alpha1.ComponentSourceUnion{ + GitSource: &appstudiov1alpha1.GitSource{ + URL: "url", + }, + }, + }, + Route: "route1", + TargetPort: 1111, + Env: env, + Resources: originalResources, + }, + }, + updateExpected: false, + }, + { + // no update to the component model because replica value has not changed + name: "replica test: comp.Spec.Replicas is 1, attribute deploy/replicas: 1. Replicas should be 1, no update", + components: []devfileAPIV1.Component{ + { + Name: "component1", + Attributes: envAttributes.PutInteger(devfilePkg.ReplicaKey, 1), + ComponentUnion: devfileAPIV1.ComponentUnion{ + Kubernetes: &devfileAPIV1.KubernetesComponent{}, + }, + }, + }, + component: appstudiov1alpha1.Component{ + Spec: appstudiov1alpha1.ComponentSpec{ + ComponentName: "componentName", + Application: "applicationName", + Replicas: &oneReplica, + Source: appstudiov1alpha1.ComponentSource{ + ComponentSourceUnion: appstudiov1alpha1.ComponentSourceUnion{ + GitSource: &appstudiov1alpha1.GitSource{ + URL: "url", + }, + }, + }, + Route: "route1", + TargetPort: 1111, + Env: env, + Resources: originalResources, + }, + }, + updateExpected: false, }, { name: "two kubernetes components", @@ -716,13 +1006,14 @@ func TestUpdateComponentDevfileModel(t *testing.T) { }, }, Route: "route1", - Replicas: &numReplica, + Replicas: &oneReplica, TargetPort: 1111, Env: env, Resources: originalResources, }, }, updateExpected: true, + wantReplica: &oneReplica, }, { name: "Component with envFrom component - should error out as it's not supported right now", @@ -825,13 +1116,14 @@ func TestUpdateComponentDevfileModel(t *testing.T) { }, }, Route: "route1", - Replicas: &numReplica, + Replicas: &oneReplica, TargetPort: 1111, Env: env, Resources: originalResources, }, }, updateExpected: true, + wantReplica: &oneReplica, }, { name: "devfile with invalid components, error out when trying to update devfile's Dockerfile uri", @@ -875,7 +1167,7 @@ func TestUpdateComponentDevfileModel(t *testing.T) { }, }, Route: "route1", - Replicas: &numReplica, + Replicas: &oneReplica, TargetPort: 1111, Env: env, Resources: originalResources, @@ -913,7 +1205,7 @@ func TestUpdateComponentDevfileModel(t *testing.T) { // it has been updated checklist := updateChecklist{ route: tt.component.Spec.Route, - replica: *tt.component.Spec.Replicas, + replica: *tt.wantReplica, port: tt.component.Spec.TargetPort, env: tt.component.Spec.Env, resources: tt.component.Spec.Resources, diff --git a/pkg/devfile/devfile.go b/pkg/devfile/devfile.go index 402b9e251..d3fbf48b2 100644 --- a/pkg/devfile/devfile.go +++ b/pkg/devfile/devfile.go @@ -124,14 +124,27 @@ func GetResourceFromDevfile(log logr.Logger, devfileData data.DevfileData, deplo } if len(resources.Deployments) > 0 { - // update for replica - currentReplica := int32(component.Attributes.GetNumber(ReplicaKey, &err)) - if err != nil { - if _, ok := err.(*attributes.KeyNotFoundError); !ok { + // need to pass in a new error holder, otherwise previous errors will persist and give a false error if key is found + var replicaErr error + currentReplica := int32(component.Attributes.GetNumber(ReplicaKey, &replicaErr)) + if replicaErr != nil { + if _, ok := replicaErr.(*attributes.KeyNotFoundError); !ok { return parser.KubernetesResources{}, err + } else { + // check the deployment to see what value is set + replica := resources.Deployments[0].Spec.Replicas + if replica != nil { + // if there is no component attribute, we will use the value in the deployment spec + currentReplica = *replica + } else { + // default value is 1. We shouldn't hit this code path since this check is done in the update logic and will write the attribute deployment/replicas:1 + currentReplica = 1 + } } } + resources.Deployments[0].Spec.Replicas = ¤tReplica + // Set the RevisionHistoryLimit for all Deployments to 0, if it's unset // If set, leave it alone for i := range resources.Deployments { @@ -166,9 +179,7 @@ func GetResourceFromDevfile(log logr.Logger, devfileData data.DevfileData, deplo resources.Deployments[0].Spec.Template.ObjectMeta.Labels = matchLabels } - if currentReplica > 0 { - resources.Deployments[0].Spec.Replicas = ¤tReplica - } + //resources.Deployments[0].Spec.Replicas = ¤tReplica if len(resources.Deployments[0].Spec.Template.Spec.Containers) > 0 { if image != "" { diff --git a/pkg/devfile/devfile_test.go b/pkg/devfile/devfile_test.go index 84f02da3f..02ca51a86 100644 --- a/pkg/devfile/devfile_test.go +++ b/pkg/devfile/devfile_test.go @@ -1210,6 +1210,7 @@ components: deployment/container-port: 1111 deployment/storageLimit: 401Mi deployment/storageRequest: 201Mi + deployment/replicas: 1 kubernetes: deployByDefault: false endpoints: @@ -1307,6 +1308,7 @@ components: deployment/container-port: 1111 deployment/storageLimit: 401Mi deployment/storageRequest: 201Mi + deployment/replicas: 5 kubernetes: deployByDefault: false endpoints: @@ -1436,7 +1438,7 @@ components: name: deploy-sample spec: revisionHistoryLimit: 5 - replicas: 1 + replicas: 5 selector: matchLabels: app.kubernetes.io/instance: component-sample @@ -1530,6 +1532,7 @@ components: - attributes: api.devfile.io/k8sLikeComponent-originalURI: deploy.yaml deployment/container-port: 5566 + deployment/replicas: 5 kubernetes: deployByDefault: false endpoints: @@ -2524,7 +2527,7 @@ schemaVersion: 2.2.0` }, Spec: appsv1.DeploymentSpec{ RevisionHistoryLimit: &revHistoryLimit, - Replicas: &replicaUpdated, + Replicas: &replica, // value from component attribute deployment/replicas Selector: &metav1.LabelSelector{ MatchLabels: map[string]string{ "app.kubernetes.io/instance": "component-sample", @@ -2714,7 +2717,7 @@ schemaVersion: 2.2.0` }, Spec: appsv1.DeploymentSpec{ RevisionHistoryLimit: &setRevHistoryLimit, - Replicas: &replicaUpdated, + Replicas: &replica, // replica value from deployment spec Selector: &metav1.LabelSelector{ MatchLabels: map[string]string{ "app.kubernetes.io/instance": "component-sample", @@ -3397,7 +3400,7 @@ schemaVersion: 2.2.0` }, Spec: appsv1.DeploymentSpec{ RevisionHistoryLimit: &revHistoryLimit, - Replicas: &replicaUpdated, + Replicas: &replica, //replica from component attribute Selector: &metav1.LabelSelector{ MatchLabels: map[string]string{ "app.kubernetes.io/instance": "component-sample",