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 <umohnani@redhat.com>

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)
 	}

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)
 

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))

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
+}