From 6b7917bcd6bbdf9fe6ce3892dcfe56f8fd5239d8 Mon Sep 17 00:00:00 2001 From: Oliver Walsh Date: Thu, 28 Nov 2024 15:56:35 +0000 Subject: [PATCH 1/3] Set a namespace default nodeSelector for nodeSelector kuttl test --- .../tests/ctlplane-nodeselectors/01-deploy-openstack.yaml | 2 ++ .../ctlplane-nodeselectors/02-assert-nodeselector.yaml | 2 +- .../ctlplane-nodeselectors/03-update-nodeselector.yaml | 6 ------ .../ctlplane-nodeselectors/04-assert-nodeselector.yaml | 2 +- tests/kuttl/tests/ctlplane-nodeselectors/05-cleanup.yaml | 1 + 5 files changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/kuttl/tests/ctlplane-nodeselectors/01-deploy-openstack.yaml b/tests/kuttl/tests/ctlplane-nodeselectors/01-deploy-openstack.yaml index bfffa10c8..ae7856cd3 100644 --- a/tests/kuttl/tests/ctlplane-nodeselectors/01-deploy-openstack.yaml +++ b/tests/kuttl/tests/ctlplane-nodeselectors/01-deploy-openstack.yaml @@ -1,5 +1,7 @@ apiVersion: kuttl.dev/v1beta1 kind: TestStep commands: + - script: | + oc annotate namespace $NAMESPACE openshift.io/node-selector="beta.kubernetes.io/os=linux" - script: | oc kustomize ../../../../config/samples/nodeselectors/global | oc apply -n $NAMESPACE -f - diff --git a/tests/kuttl/tests/ctlplane-nodeselectors/02-assert-nodeselector.yaml b/tests/kuttl/tests/ctlplane-nodeselectors/02-assert-nodeselector.yaml index dfb3f117a..bf160a363 100644 --- a/tests/kuttl/tests/ctlplane-nodeselectors/02-assert-nodeselector.yaml +++ b/tests/kuttl/tests/ctlplane-nodeselectors/02-assert-nodeselector.yaml @@ -3,7 +3,7 @@ kind: TestAssert commands: - script: | echo "Checking all pods have expected nodeselector" - EXPECTED_NODE_SELECTOR="node-role.kubernetes.io/worker:" + EXPECTED_NODE_SELECTOR="beta.kubernetes.io/os:linux node-role.kubernetes.io/worker:" BAD_OR_MISSING_NODE_SELECTOR=$(oc get pods -n $NAMESPACE -l service!=dnsmasq -o=go-template --template='{{ range .items }}{{ .metadata.name}}: {{ .spec.nodeSelector }}{{"\n"}}{{ end }}' | grep -v 'ovn-controller-.*-config' | sed -e '\!map\['"$EXPECTED_NODE_SELECTOR"'\]$!d') BAD_OR_MISSING_NODE_SELECTOR_COUNT=$(echo -n "$BAD_OR_MISSING_NODE_SELECTOR" | wc -l) if [ $BAD_OR_MISSING_NODE_SELECTOR_COUNT -ne 0 ]; then diff --git a/tests/kuttl/tests/ctlplane-nodeselectors/03-update-nodeselector.yaml b/tests/kuttl/tests/ctlplane-nodeselectors/03-update-nodeselector.yaml index 2d50bda2e..9bd994514 100644 --- a/tests/kuttl/tests/ctlplane-nodeselectors/03-update-nodeselector.yaml +++ b/tests/kuttl/tests/ctlplane-nodeselectors/03-update-nodeselector.yaml @@ -2,12 +2,6 @@ apiVersion: kuttl.dev/v1beta1 kind: TestStep timeout: 60 commands: - - script: | - oc patch dnsmasq -n $NAMESPACE dnsmasq --type='json' -p='[{ - "op": "replace", - "path": "/spec/nodeSelector", - "value": {"kubernetes.io/os":"linux"} - }]' - script: | oc patch openstackcontrolplane -n $NAMESPACE openstack --type='json' -p='[{ "op": "replace", diff --git a/tests/kuttl/tests/ctlplane-nodeselectors/04-assert-nodeselector.yaml b/tests/kuttl/tests/ctlplane-nodeselectors/04-assert-nodeselector.yaml index 031ccccc3..3df8c4772 100644 --- a/tests/kuttl/tests/ctlplane-nodeselectors/04-assert-nodeselector.yaml +++ b/tests/kuttl/tests/ctlplane-nodeselectors/04-assert-nodeselector.yaml @@ -3,7 +3,7 @@ kind: TestAssert commands: - script: | echo "Checking all running pods have new nodeselector" - EXPECTED_NODE_SELECTOR="kubernetes.io/os:linux" + EXPECTED_NODE_SELECTOR="beta.kubernetes.io/os:linux kubernetes.io/os:linux" BAD_OR_MISSING_NODE_SELECTOR=$(oc get pods -n $NAMESPACE -l service!=dnsmasq --field-selector=status.phase=Running -o=go-template --template='{{ range .items }}{{ .metadata.name}}: {{ .spec.nodeSelector }}{{"\n"}}{{ end }}' | grep -v 'ovn-controller-.*-config' | sed -e '\!map\['"$EXPECTED_NODE_SELECTOR"'\]$!d') BAD_OR_MISSING_NODE_SELECTOR_COUNT=$(echo -n "$BAD_OR_MISSING_NODE_SELECTOR" | wc -l) if [ $BAD_OR_MISSING_NODE_SELECTOR_COUNT -ne 0 ]; then diff --git a/tests/kuttl/tests/ctlplane-nodeselectors/05-cleanup.yaml b/tests/kuttl/tests/ctlplane-nodeselectors/05-cleanup.yaml index 6b4992512..722a0e8b1 100644 --- a/tests/kuttl/tests/ctlplane-nodeselectors/05-cleanup.yaml +++ b/tests/kuttl/tests/ctlplane-nodeselectors/05-cleanup.yaml @@ -6,6 +6,7 @@ delete: name: openstack commands: - script: | + oc annotate namespace $NAMESPACE openshift.io/node-selector- oc delete --ignore-not-found=true -n $NAMESPACE pvc \ srv-swift-storage-0 oc delete secret --ignore-not-found=true combined-ca-bundle -n $NAMESPACE From 5b6b54b77bfce3291420faef1b8a2d69920691f4 Mon Sep 17 00:00:00 2001 From: Oliver Walsh Date: Thu, 5 Dec 2024 16:19:53 +0000 Subject: [PATCH 2/3] Store hash to determine when pod spec changes Comparing the desired pod spec to the current pod spec is not reliable as the spec can be mutated by k8s e.g to add volume mount for a service account or to add the default nodeselector from the namespace/cluster on OpenShift Instead store a hash of the original pod spec and compare this to the hash of the current desired pod spec to determine if the spec has changed. --- .../client.openstack.org_openstackclients.yaml | 4 ++++ apis/client/v1beta1/openstackclient_types.go | 3 +++ apis/client/v1beta1/zz_generated.deepcopy.go | 7 +++++++ .../client.openstack.org_openstackclients.yaml | 4 ++++ .../client/openstackclient_controller.go | 17 +++++++++++------ 5 files changed, 29 insertions(+), 6 deletions(-) diff --git a/apis/bases/client.openstack.org_openstackclients.yaml b/apis/bases/client.openstack.org_openstackclients.yaml index 96dd6f0ac..7e6d21ca9 100644 --- a/apis/bases/client.openstack.org_openstackclients.yaml +++ b/apis/bases/client.openstack.org_openstackclients.yaml @@ -82,6 +82,10 @@ spec: - type type: object type: array + hash: + additionalProperties: + type: string + type: object observedGeneration: format: int64 type: integer diff --git a/apis/client/v1beta1/openstackclient_types.go b/apis/client/v1beta1/openstackclient_types.go index f2e044574..584b4eef5 100644 --- a/apis/client/v1beta1/openstackclient_types.go +++ b/apis/client/v1beta1/openstackclient_types.go @@ -68,6 +68,9 @@ type OpenStackClientStatus struct { //ObservedGeneration - the most recent generation observed for this object. ObservedGeneration int64 `json:"observedGeneration,omitempty"` + + // Map of hashes to track e.g. pod spec + Hash map[string]string `json:"hash,omitempty"` } // +kubebuilder:object:root=true diff --git a/apis/client/v1beta1/zz_generated.deepcopy.go b/apis/client/v1beta1/zz_generated.deepcopy.go index 010dfef40..fcd81f6a0 100644 --- a/apis/client/v1beta1/zz_generated.deepcopy.go +++ b/apis/client/v1beta1/zz_generated.deepcopy.go @@ -163,6 +163,13 @@ func (in *OpenStackClientStatus) DeepCopyInto(out *OpenStackClientStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.Hash != nil { + in, out := &in.Hash, &out.Hash + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OpenStackClientStatus. diff --git a/config/crd/bases/client.openstack.org_openstackclients.yaml b/config/crd/bases/client.openstack.org_openstackclients.yaml index 96dd6f0ac..7e6d21ca9 100644 --- a/config/crd/bases/client.openstack.org_openstackclients.yaml +++ b/config/crd/bases/client.openstack.org_openstackclients.yaml @@ -82,6 +82,10 @@ spec: - type type: object type: array + hash: + additionalProperties: + type: string + type: object observedGeneration: format: int64 type: integer diff --git a/controllers/client/openstackclient_controller.go b/controllers/client/openstackclient_controller.go index d8d2eafc1..9bf5d3e85 100644 --- a/controllers/client/openstackclient_controller.go +++ b/controllers/client/openstackclient_controller.go @@ -307,16 +307,19 @@ func (r *OpenStackClientReconciler) Reconcile(ctx context.Context, req ctrl.Requ spec := openstackclient.ClientPodSpec(ctx, instance, helper, configVarsHash) + podSpecHash, err := util.ObjectHash(spec) + if err != nil { + return ctrl.Result{}, err + } + + podSpecHashName := "podSpec" + op, err := controllerutil.CreateOrPatch(ctx, r.Client, osclient, func() error { isPodUpdate := !osclient.ObjectMeta.CreationTimestamp.IsZero() - if !isPodUpdate { + currentPodSpecHash := instance.Status.Hash[podSpecHashName] + if !isPodUpdate || currentPodSpecHash != podSpecHash { osclient.Spec = spec - } else { - osclient.Spec.Containers[0].Env = spec.Containers[0].Env - osclient.Spec.NodeSelector = spec.NodeSelector - osclient.Spec.Containers[0].Image = instance.Spec.ContainerImage } - osclient.Labels = util.MergeStringMaps(osclient.Labels, clientLabels) err = controllerutil.SetControllerReference(instance, osclient, r.Scheme) @@ -360,6 +363,8 @@ func (r *OpenStackClientReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, err } + instance.Status.Hash, _ = util.SetHash(instance.Status.Hash, podSpecHashName, podSpecHash) + if op != controllerutil.OperationResultNone { util.LogForObject( helper, From eaa330cc51c7ec6affe139101b54e66bb47ef70e Mon Sep 17 00:00:00 2001 From: Oliver Walsh Date: Thu, 5 Dec 2024 16:20:26 +0000 Subject: [PATCH 3/3] [crd-schema-checker] disable NoMaps validator Related: https://issues.redhat.com/browse/OSPRH-12254 --- hack/crd-schema-checker.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/hack/crd-schema-checker.sh b/hack/crd-schema-checker.sh index 3a252acd6..ee105c734 100755 --- a/hack/crd-schema-checker.sh +++ b/hack/crd-schema-checker.sh @@ -3,6 +3,13 @@ set -euxo pipefail CHECKER=$INSTALL_DIR/crd-schema-checker +DISABLED_VALIDATORS=NoMaps # TODO: https://issues.redhat.com/browse/OSPRH-12254 + +CHECKER_ARGS="" +if [[ ${DISABLED_VALIDATORS:+x} ]]; then + CHECKER_ARGS="$CHECKER_ARGS --disabled-validators $DISABLED_VALIDATORS" +fi + TMP_DIR=$(mktemp -d) function cleanup { @@ -16,6 +23,7 @@ for crd in config/crd/bases/*.yaml; do mkdir -p "$(dirname "$TMP_DIR/$crd")" if git show "$BASE_REF:$crd" > "$TMP_DIR/$crd"; then $CHECKER check-manifests \ + $CHECKER_ARGS \ --existing-crd-filename="$TMP_DIR/$crd" \ --new-crd-filename="$crd" fi