From 5dd01756d443784b3dd2c99963278178cba84a4d Mon Sep 17 00:00:00 2001 From: Urvashi Date: Wed, 22 Jan 2025 14:25:57 -0500 Subject: [PATCH] Ensure build job is deleted when rebuild is triggered Ensure that the build job under the MOSB is deleted before deleting the MOSB when the rebuild annotation is added to the MOSC. This ensures proper cleanup so that the build can start again. Signed-off-by: Urvashi --- pkg/controller/build/reconciler.go | 5 ++ test/e2e-ocl/helpers_test.go | 18 ++++-- test/e2e-ocl/onclusterlayering_test.go | 82 +++++++++++++++++++++++++- test/helpers/utils.go | 15 +++++ 4 files changed, 112 insertions(+), 8 deletions(-) diff --git a/pkg/controller/build/reconciler.go b/pkg/controller/build/reconciler.go index db8c7e0b8f..6f4d3244d7 100644 --- a/pkg/controller/build/reconciler.go +++ b/pkg/controller/build/reconciler.go @@ -127,6 +127,11 @@ func (b *buildReconciler) rebuildMachineOSConfig(ctx context.Context, mosc *mcfg return ignoreErrIsNotFound(fmt.Errorf("cannot rebuild MachineOSConfig %q: %w", mosc.Name, err)) } + // delete the build objects before deleting the mosb + // this can e removed once we have proper owner references + if err := b.deleteBuilderForMachineOSBuild(ctx, mosb); err != nil { + return fmt.Errorf("could not delete builder for MachineOSBuild %q: %w", mosb.Name, err) + } if err := b.deleteMachineOSBuild(ctx, mosb); err != nil { return fmt.Errorf("could not delete MachineOSBuild %q for MachineOSConfig %q: %w", mosb.Name, mosc.Name, err) } diff --git a/test/e2e-ocl/helpers_test.go b/test/e2e-ocl/helpers_test.go index b30f96bac0..1c14c1d99c 100644 --- a/test/e2e-ocl/helpers_test.go +++ b/test/e2e-ocl/helpers_test.go @@ -208,7 +208,7 @@ func createSecret(t *testing.T, cs *framework.ClientSet, secret *corev1.Secret) // Copies the global pull secret from openshift-config/pull-secret into the MCO // namespace so that it can be used by the build processes. -func copyGlobalPullSecret(t *testing.T, cs *framework.ClientSet) func() { +func copyGlobalPullSecret(t *testing.T, cs *framework.ClientSet, useIncorrectSecret bool) func() { src := metav1.ObjectMeta{ Name: "pull-secret", Namespace: "openshift-config", @@ -219,7 +219,7 @@ func copyGlobalPullSecret(t *testing.T, cs *framework.ClientSet) func() { Namespace: ctrlcommon.MCONamespace, } - return cloneSecret(t, cs, src, dst) + return cloneSecret(t, cs, src, dst, useIncorrectSecret) } // Computes the name of the currently-running MachineOSBuild given a MachineConfigPool and MachineOSConfig. @@ -717,7 +717,7 @@ func convertFilesFromContainerImageToBytesMap(t *testing.T, pullspec, containerF // cannot be found, calls t.Skip() to skip the test. // // Registers and returns a cleanup function to remove the certificate(s) after test completion. -func copyEntitlementCerts(t *testing.T, cs *framework.ClientSet) func() { +func copyEntitlementCerts(t *testing.T, cs *framework.ClientSet, useIncorrectSecret bool) func() { src := metav1.ObjectMeta{ Name: "etc-pki-entitlement", Namespace: "openshift-config-managed", @@ -730,7 +730,7 @@ func copyEntitlementCerts(t *testing.T, cs *framework.ClientSet) func() { _, err := cs.CoreV1Interface.Secrets(src.Namespace).Get(context.TODO(), src.Name, metav1.GetOptions{}) if err == nil { - return cloneSecret(t, cs, src, dst) + return cloneSecret(t, cs, src, dst, useIncorrectSecret) } if k8serrors.IsNotFound(err) { @@ -788,7 +788,7 @@ func injectYumRepos(t *testing.T, cs *framework.ClientSet) func() { // Clones a given secret from a given namespace into the MCO namespace. // Registers and returns a cleanup function to delete the secret upon test // completion. -func cloneSecret(t *testing.T, cs *framework.ClientSet, src, dst metav1.ObjectMeta) func() { +func cloneSecret(t *testing.T, cs *framework.ClientSet, src, dst metav1.ObjectMeta, useIncorrectSecret bool) func() { secret, err := cs.CoreV1Interface.Secrets(src.Namespace).Get(context.TODO(), src.Name, metav1.GetOptions{}) require.NoError(t, err) @@ -804,6 +804,14 @@ func cloneSecret(t *testing.T, cs *framework.ClientSet, src, dst metav1.ObjectMe Type: secret.Type, } + // If the test is configured to use an incorrect secret, then we will + // replace the secret data with a simple string. + if useIncorrectSecret { + secretCopy.Data = map[string][]byte{ + "foo": []byte("bar"), + } + } + cleanup := createSecret(t, cs, secretCopy) t.Logf("Cloned \"%s/%s\" to \"%s/%s\"", src.Namespace, src.Name, dst.Namespace, dst.Name) diff --git a/test/e2e-ocl/onclusterlayering_test.go b/test/e2e-ocl/onclusterlayering_test.go index 9de10b4b32..bafd14a805 100644 --- a/test/e2e-ocl/onclusterlayering_test.go +++ b/test/e2e-ocl/onclusterlayering_test.go @@ -95,6 +95,9 @@ type onClusterLayeringTestOpts struct { // Add Extensions for testing useExtensions bool + + // Use incorrect secrets + useIncorrectSecrets bool } func TestOnClusterLayeringOnOKD(t *testing.T) { @@ -276,6 +279,59 @@ func TestMachineOSConfigChangeRestartsBuild(t *testing.T) { require.NoError(t, err) } +// This test verifies that if the rebuild annotation is added to a given MachineOSConfig, that +// the build is restarted +func TestRebuildAnnotationRestartsBuild(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + cs := framework.NewClientSet("") + + mosc := prepareForOnClusterLayeringTest(t, cs, onClusterLayeringTestOpts{ + poolName: layeredMCPName, + customDockerfiles: map[string]string{ + layeredMCPName: cowsayDockerfile, + }, + }) + + createMachineOSConfig(t, cs, mosc) + + mcp, err := cs.MachineconfigurationV1Interface.MachineConfigPools().Get(ctx, layeredMCPName, metav1.GetOptions{}) + require.NoError(t, err) + + firstMosb := buildrequest.NewMachineOSBuildFromAPIOrDie(ctx, cs.GetKubeclient(), mosc, mcp) + + // First, we get a MachineOSBuild started as usual. + waitForBuildToStart(t, cs, firstMosb) + + t.Logf("Initial build has started, waiting for it to fail...") + + waitForBuildToFail(t, cs, firstMosb) + + // Fix the global pull secret + t.Logf("Initial build has failed, updating global pull secret...") + + err = cs.CoreV1Interface.Secrets(ctrlcommon.MCONamespace).Delete(context.TODO(), globalPullSecretCloneName, metav1.DeleteOptions{}) + require.NoError(t, err) + copyGlobalPullSecret(t, cs, false) + + _ = helpers.SetRebuildAnnotationOnMachineOSConfig(ctx, t, cs.GetMcfgclient(), mosc) + + kubeassert := helpers.AssertClientSet(t, cs).WithContext(ctx) + + assertBuildObjectsAreCreated(t, kubeassert, firstMosb) + + t.Logf("Annotation is updated, waiting for new build %s to start", firstMosb.Name) + + // Wait for the build to start. + waitForBuildToStart(t, cs, firstMosb) + + t.Logf("Waiting for MachineOSBuild %s to be successful", firstMosb.Name) + + // Wait for the build to be successful. + waitForBuildToComplete(t, cs, firstMosb) +} + // This test verifies that a change to the MachineConfigPool, such as the // presence of a new rendered MachineConfig, will halt the currently running // build, replacing it with a new build instead. @@ -931,7 +987,7 @@ func waitForBuildToComplete(t *testing.T, cs *framework.ClientSet, startedBuild start := time.Now() kubeassert := helpers.AssertClientSet(t, cs).WithContext(ctx) - kubeassert.Eventually().MachineOSBuildIsSuccessful(startedBuild) //foo + kubeassert.Eventually().MachineOSBuildIsSuccessful(startedBuild) t.Logf("MachineOSBuild %s successful after %s", startedBuild.Name, time.Since(start)) assertBuildObjectsAreDeleted(t, kubeassert.Eventually(), startedBuild) t.Logf("Build objects deleted after %s", time.Since(start)) @@ -942,6 +998,26 @@ func waitForBuildToComplete(t *testing.T, cs *framework.ClientSet, startedBuild return mosb } +func waitForBuildToFail(t *testing.T, cs *framework.ClientSet, startedBuild *mcfgv1alpha1.MachineOSBuild) *mcfgv1alpha1.MachineOSBuild { + t.Helper() + + t.Logf("Waiting for MachineOSBuild %s to complete", startedBuild.Name) + + ctx, cancel := context.WithTimeout(context.Background(), time.Minute*20) + defer cancel() + + start := time.Now() + + kubeassert := helpers.AssertClientSet(t, cs).WithContext(ctx) + kubeassert.Eventually().MachineOSBuildIsFailure(startedBuild) + t.Logf("MachineOSBuild %s failed after %s", startedBuild.Name, time.Since(start)) + + mosb, err := cs.MachineconfigurationV1alpha1Interface.MachineOSBuilds().Get(ctx, startedBuild.Name, metav1.GetOptions{}) + require.NoError(t, err) + + return mosb +} + // Validates that the build job is configured correctly. In this case, // "correctly" means that it has the correct container images. Future // assertions could include things like ensuring that the proper volume mounts @@ -988,7 +1064,7 @@ func prepareForOnClusterLayeringTest(t *testing.T, cs *framework.ClientSet, test // If the test requires RHEL entitlements, clone them from // "etc-pki-entitlement" in the "openshift-config-managed" namespace. if testOpts.useEtcPkiEntitlement { - copyEntitlementCerts(t, cs) + copyEntitlementCerts(t, cs, false) } // If the test requires /etc/yum.repos.d and /etc/pki/rpm-gpg, pull a Centos @@ -1011,7 +1087,7 @@ func prepareForOnClusterLayeringTest(t *testing.T, cs *framework.ClientSet, test pushSecretName, finalPullspec, _ := setupImageStream(t, cs, imagestreamObjMeta) - copyGlobalPullSecret(t, cs) + copyGlobalPullSecret(t, cs, testOpts.useIncorrectSecrets) if testOpts.targetNode != nil { makeIdempotentAndRegister(t, helpers.CreatePoolWithNode(t, cs, testOpts.poolName, *testOpts.targetNode)) diff --git a/test/helpers/utils.go b/test/helpers/utils.go index 5f934bac40..f60c5548c5 100644 --- a/test/helpers/utils.go +++ b/test/helpers/utils.go @@ -31,6 +31,7 @@ import ( mcfgv1 "github.com/openshift/api/machineconfiguration/v1" machineClientv1beta1 "github.com/openshift/client-go/machine/clientset/versioned/typed/machine/v1beta1" "github.com/openshift/machine-config-operator/pkg/apihelpers" + buildConstants "github.com/openshift/machine-config-operator/pkg/controller/build/constants" "github.com/openshift/machine-config-operator/pkg/daemon/constants" "github.com/openshift/machine-config-operator/pkg/daemon/osrelease" "github.com/openshift/machine-config-operator/test/framework" @@ -1755,3 +1756,17 @@ func SetContainerfileContentsOnMachineOSConfig(ctx context.Context, t *testing.T return apiMosc } + +func SetRebuildAnnotationOnMachineOSConfig(ctx context.Context, t *testing.T, mcfgclient mcfgclientset.Interface, mosc *mcfgv1alpha1.MachineOSConfig) *mcfgv1alpha1.MachineOSConfig { + t.Helper() + + apiMosc, err := mcfgclient.MachineconfigurationV1alpha1().MachineOSConfigs().Get(ctx, mosc.Name, metav1.GetOptions{}) + require.NoError(t, err) + + apiMosc.Annotations[buildConstants.RebuildMachineOSConfigAnnotationKey] = "" + + apiMosc, err = mcfgclient.MachineconfigurationV1alpha1().MachineOSConfigs().Update(ctx, apiMosc, metav1.UpdateOptions{}) + require.NoError(t, err) + + return apiMosc +}