Skip to content

Commit 53374f3

Browse files
carboninopenshift-merge-bot[bot]
carbonin
authored and
openshift-merge-bot[bot]
committed
Label referenced objects for backup even if the cluster is installed
Previously if/when IBIO was upgraded to include changes to add backup labels the labels would not be added for resources referenced by already installed clusters. To allow for this, the controller now needs to get the ClusterDeployment earlier (to find the referenced resources). To allow the conditions to still be set as before in cases where the ClusterDeployment is missing or the reference is not set, Reconcile specifically does not return when ClusterDeployment can't be found in this new, earlier, getCD call error case. This also handles labeling created resources immediately by adding the backup label to them when they are created in the credentials package.
1 parent ea06974 commit 53374f3

4 files changed

Lines changed: 180 additions & 35 deletions

File tree

‎controllers/imageclusterinstall_controller.go‎

Lines changed: 50 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,17 @@ func (r *ImageClusterInstallReconciler) Reconcile(ctx context.Context, req ctrl.
147147
return res, err
148148
}
149149

150+
var cd *hivev1.ClusterDeployment
151+
if ici.Spec.ClusterDeploymentRef != nil && ici.Spec.ClusterDeploymentRef.Name != "" {
152+
var err error
153+
cd, err = r.getCD(ctx, ici)
154+
if err != nil {
155+
log.Error(err)
156+
}
157+
}
158+
159+
r.labelReferencedObjectsForBackup(ctx, log, ici, cd)
160+
150161
// Nothing to do if the installation is complete
151162
if InstallationCompleted(ici) {
152163
return ctrl.Result{}, nil
@@ -199,7 +210,7 @@ func (r *ImageClusterInstallReconciler) Reconcile(ctx context.Context, req ctrl.
199210
// when the problem is resolved.
200211
// - ConfigurationFailed: sets this reason when AutomatedCleaningMode cannot be modified in BMH.
201212
cond.Reason = v1alpha1.ConfigurationPendingReason
202-
cd, bmh, err := r.validateConfiguration(ctx, ici, &cond, log)
213+
bmh, err := r.validateConfiguration(ctx, ici, cd, &cond, log)
203214
if cd == nil || bmh == nil || err != nil {
204215
return ctrl.Result{}, err
205216
}
@@ -231,8 +242,6 @@ func (r *ImageClusterInstallReconciler) Reconcile(ctx context.Context, req ctrl.
231242
return res, err
232243
}
233244

234-
r.labelReferencedObjectsForBackup(ctx, log, ici, cd)
235-
236245
// 4. Host configuration phase
237246
// Possible reasons for not meeting requirements and exiting reconcile:
238247
// - HostConfigurationPending: sets this reason in following scenarios:
@@ -261,34 +270,33 @@ func GetClusterConfigDir(namespacesDir, namespace, uid string) string {
261270
func (r *ImageClusterInstallReconciler) validateConfiguration(
262271
ctx context.Context,
263272
ici *v1alpha1.ImageClusterInstall,
273+
cd *hivev1.ClusterDeployment,
264274
cond *hivev1.ClusterInstallCondition,
265275
log logrus.FieldLogger,
266-
) (*hivev1.ClusterDeployment, *bmh_v1alpha1.BareMetalHost, error) {
267-
268-
if ici.Spec.ClusterDeploymentRef == nil || ici.Spec.ClusterDeploymentRef.Name == "" {
269-
cond.Message = "ClusterDeploymentRef is unset"
270-
log.Error(errors.New(cond.Message))
271-
return nil, nil, nil
272-
}
276+
) (*bmh_v1alpha1.BareMetalHost, error) {
277+
if cd == nil {
278+
if ici.Spec.ClusterDeploymentRef == nil || ici.Spec.ClusterDeploymentRef.Name == "" {
279+
cond.Message = "ClusterDeploymentRef is unset"
280+
log.Error(errors.New(cond.Message))
281+
return nil, nil
282+
}
273283

274-
cd, err := r.getCD(ctx, ici)
275-
if err != nil {
284+
// in this case error would have been logged when cd couldn't be queried earlier in Reconcile so just set the Message here
276285
cond.Message = fmt.Sprintf("failed to get ClusterDeployment %s/%s", ici.Namespace, ici.Spec.ClusterDeploymentRef.Name)
277-
log.Error(err)
278-
return nil, nil, nil
286+
return nil, nil
279287
}
280288

281289
if ici.Spec.BareMetalHostRef == nil || ici.Spec.BareMetalHostRef.Name == "" {
282290
cond.Message = "BareMetalHostRef is unset"
283291
log.Error(errors.New(cond.Message))
284-
return nil, nil, nil
292+
return nil, nil
285293
}
286294

287295
bmh, err := r.getBMH(ctx, ici.Spec.BareMetalHostRef)
288296
if err != nil {
289297
cond.Message = fmt.Sprintf("failed to get BareMetalHost %s/%s", ici.Spec.BareMetalHostRef.Namespace, ici.Spec.BareMetalHostRef.Name)
290298
log.Error(err)
291-
return nil, nil, nil
299+
return nil, nil
292300
}
293301

294302
// AutomatedCleaningMode is set at the beginning of this flow because we don't want ironic to format the disk
@@ -300,11 +308,11 @@ func (r *ImageClusterInstallReconciler) validateConfiguration(
300308
cond.Reason = v1alpha1.ConfigurationFailedReason
301309
cond.Message = fmt.Sprintf("failed to disable automated cleaning mode for BareMetalHost %s/%s", bmh.Namespace, bmh.Name)
302310
log.WithError(err).Error(cond.Message)
303-
return nil, nil, err
311+
return nil, err
304312
}
305313
}
306314

307-
return cd, bmh, nil
315+
return bmh, nil
308316
}
309317

310318
func (r *ImageClusterInstallReconciler) validateHost(
@@ -933,19 +941,6 @@ func (r *ImageClusterInstallReconciler) labelDataImageForBackup(ctx context.Cont
933941
}
934942

935943
func (r *ImageClusterInstallReconciler) labelReferencedObjectsForBackup(ctx context.Context, log logrus.FieldLogger, ici *v1alpha1.ImageClusterInstall, cd *hivev1.ClusterDeployment) {
936-
if ici.Spec.ClusterMetadata != nil {
937-
kubeconfigKey := types.NamespacedName{Name: ici.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name, Namespace: ici.Namespace}
938-
if err := r.labelSecretForBackup(ctx, kubeconfigKey); err != nil {
939-
log.WithError(err).Errorf("failed to label Secret %s for backup", kubeconfigKey)
940-
}
941-
if ici.Spec.ClusterMetadata.AdminPasswordSecretRef != nil {
942-
passwordKey := types.NamespacedName{Name: ici.Spec.ClusterMetadata.AdminPasswordSecretRef.Name, Namespace: ici.Namespace}
943-
if err := r.labelSecretForBackup(ctx, passwordKey); err != nil {
944-
log.WithError(err).Errorf("failed to label Secret %s for backup", passwordKey)
945-
}
946-
}
947-
}
948-
949944
if ici.Spec.CABundleRef != nil {
950945
caBundleKey := types.NamespacedName{Name: ici.Spec.CABundleRef.Name, Namespace: ici.Namespace}
951946
if err := r.labelConfigMapForBackup(ctx, caBundleKey); err != nil {
@@ -960,10 +955,17 @@ func (r *ImageClusterInstallReconciler) labelReferencedObjectsForBackup(ctx cont
960955
}
961956
}
962957

963-
if cd.Spec.PullSecretRef != nil {
964-
psKey := types.NamespacedName{Name: cd.Spec.PullSecretRef.Name, Namespace: cd.Namespace}
965-
if err := r.labelSecretForBackup(ctx, psKey); err != nil {
966-
log.WithError(err).Errorf("failed to label Secret %s for backup", psKey)
958+
if cd != nil {
959+
reconfigKey := types.NamespacedName{Name: credentials.SeedReconfigurationSecretName(cd.Name), Namespace: cd.Namespace}
960+
if err := r.labelSecretForBackup(ctx, reconfigKey); err != nil {
961+
log.WithError(err).Errorf("failed to label Secret %s for backup", reconfigKey)
962+
}
963+
964+
if cd.Spec.PullSecretRef != nil {
965+
psKey := types.NamespacedName{Name: cd.Spec.PullSecretRef.Name, Namespace: cd.Namespace}
966+
if err := r.labelSecretForBackup(ctx, psKey); err != nil {
967+
log.WithError(err).Errorf("failed to label Secret %s for backup", psKey)
968+
}
967969
}
968970
}
969971

@@ -976,6 +978,19 @@ func (r *ImageClusterInstallReconciler) labelReferencedObjectsForBackup(ctx cont
976978
log.WithError(err).Errorf("failed to label DataImage %s/%s for backup", ici.Spec.BareMetalHostRef.Namespace, ici.Spec.BareMetalHostRef.Name)
977979
}
978980
}
981+
982+
if ici.Spec.ClusterMetadata != nil {
983+
kubeconfigKey := types.NamespacedName{Name: ici.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name, Namespace: ici.Namespace}
984+
if err := r.labelSecretForBackup(ctx, kubeconfigKey); err != nil {
985+
log.WithError(err).Errorf("failed to label Secret %s for backup", kubeconfigKey)
986+
}
987+
if ici.Spec.ClusterMetadata.AdminPasswordSecretRef != nil {
988+
passwordKey := types.NamespacedName{Name: ici.Spec.ClusterMetadata.AdminPasswordSecretRef.Name, Namespace: ici.Namespace}
989+
if err := r.labelSecretForBackup(ctx, passwordKey); err != nil {
990+
log.WithError(err).Errorf("failed to label Secret %s for backup", passwordKey)
991+
}
992+
}
993+
}
979994
}
980995

981996
func (r *ImageClusterInstallReconciler) configDirs(ici *v1alpha1.ImageClusterInstall) (string, string, error) {

‎controllers/imageclusterinstall_controller_test.go‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2207,6 +2207,124 @@ var _ = Describe("Reconcile", func() {
22072207
Expect(testDataImage.GetLabels()).To(HaveKeyWithValue(backupLabel, backupLabelValue), "DataImage %s/%s missing backup label", testDataImage.Namespace, testDataImage.Name)
22082208
})
22092209

2210+
It("labels referenced resources for backup when cluster is already marked as installed", func() {
2211+
// Create BMH
2212+
bmh := bmhInState(bmh_v1alpha1.StateAvailable)
2213+
bmh.Spec.Online = true
2214+
bmh.Spec.ExternallyProvisioned = false
2215+
Expect(c.Create(ctx, bmh)).To(Succeed())
2216+
2217+
// Create DataImage
2218+
dataImage := &bmh_v1alpha1.DataImage{
2219+
ObjectMeta: metav1.ObjectMeta{
2220+
Name: bmh.Name,
2221+
Namespace: bmh.Namespace,
2222+
},
2223+
Spec: bmh_v1alpha1.DataImageSpec{
2224+
URL: imageURL(),
2225+
},
2226+
}
2227+
Expect(c.Create(ctx, dataImage)).To(Succeed())
2228+
2229+
// Create CA bundle ConfigMap
2230+
caBundleCM := &corev1.ConfigMap{
2231+
ObjectMeta: metav1.ObjectMeta{
2232+
Name: "ca-bundle",
2233+
Namespace: clusterInstallNamespace,
2234+
},
2235+
Data: map[string]string{caBundleFileName: "mycabundle"},
2236+
}
2237+
Expect(c.Create(ctx, caBundleCM)).To(Succeed())
2238+
2239+
// Create extra manifests ConfigMap
2240+
extraManifestCM := &corev1.ConfigMap{
2241+
ObjectMeta: metav1.ObjectMeta{
2242+
Name: "extra-manifest",
2243+
Namespace: clusterInstallNamespace,
2244+
},
2245+
Data: map[string]string{
2246+
"manifest.yaml": "thing: stuff",
2247+
},
2248+
}
2249+
Expect(c.Create(ctx, extraManifestCM)).To(Succeed())
2250+
2251+
// Set up ImageClusterInstall with InstallationCompleted condition set to True
2252+
clusterInstall.Spec.BareMetalHostRef = &v1alpha1.BareMetalHostReference{
2253+
Name: bmh.Name,
2254+
Namespace: bmh.Namespace,
2255+
}
2256+
clusterInstall.Spec.CABundleRef = &corev1.LocalObjectReference{
2257+
Name: "ca-bundle",
2258+
}
2259+
clusterInstall.Spec.ExtraManifestsRefs = []corev1.LocalObjectReference{
2260+
{Name: "extra-manifest"},
2261+
}
2262+
clusterInstall.Status.Conditions = []hivev1.ClusterInstallCondition{
2263+
{
2264+
Type: hivev1.ClusterInstallCompleted,
2265+
Status: corev1.ConditionTrue,
2266+
Reason: v1alpha1.InstallSucceededReason,
2267+
Message: v1alpha1.InstallSucceededMessage,
2268+
},
2269+
}
2270+
Expect(c.Create(ctx, clusterInstall)).To(Succeed())
2271+
2272+
// Set up ClusterDeployment with pull secret reference
2273+
clusterDeployment.Spec.ClusterName = "test"
2274+
clusterDeployment.Spec.BaseDomain = "example.com"
2275+
Expect(c.Create(ctx, clusterDeployment)).To(Succeed())
2276+
2277+
// Reconcile - should label resources even though InstallationCompleted is True
2278+
key := types.NamespacedName{
2279+
Namespace: clusterInstallNamespace,
2280+
Name: clusterInstallName,
2281+
}
2282+
res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: key})
2283+
Expect(err).NotTo(HaveOccurred())
2284+
Expect(res).To(Equal(ctrl.Result{}))
2285+
2286+
// Verify BMH has backup label
2287+
bmhKey := types.NamespacedName{
2288+
Namespace: bmh.Namespace,
2289+
Name: bmh.Name,
2290+
}
2291+
testBMH := &bmh_v1alpha1.BareMetalHost{}
2292+
Expect(c.Get(ctx, bmhKey, testBMH)).To(Succeed())
2293+
Expect(testBMH.GetLabels()).To(HaveKeyWithValue(backupLabel, backupLabelValue), "BMH %s/%s missing backup label", testBMH.Namespace, testBMH.Name)
2294+
2295+
// Verify DataImage has backup label
2296+
testDataImage := &bmh_v1alpha1.DataImage{}
2297+
Expect(c.Get(ctx, bmhKey, testDataImage)).To(Succeed())
2298+
Expect(testDataImage.GetLabels()).To(HaveKeyWithValue(backupLabel, backupLabelValue), "DataImage %s/%s missing backup label", testDataImage.Namespace, testDataImage.Name)
2299+
2300+
// Verify CA bundle ConfigMap has backup label
2301+
caBundleKey := types.NamespacedName{
2302+
Name: caBundleCM.Name,
2303+
Namespace: caBundleCM.Namespace,
2304+
}
2305+
testCABundleCM := &corev1.ConfigMap{}
2306+
Expect(c.Get(ctx, caBundleKey, testCABundleCM)).To(Succeed())
2307+
Expect(testCABundleCM.GetLabels()).To(HaveKeyWithValue(backupLabel, backupLabelValue), "ConfigMap %s/%s missing backup label", testCABundleCM.Namespace, testCABundleCM.Name)
2308+
2309+
// Verify extra manifest ConfigMap has backup label
2310+
extraManifestKey := types.NamespacedName{
2311+
Name: extraManifestCM.Name,
2312+
Namespace: extraManifestCM.Namespace,
2313+
}
2314+
testExtraManifestCM := &corev1.ConfigMap{}
2315+
Expect(c.Get(ctx, extraManifestKey, testExtraManifestCM)).To(Succeed())
2316+
Expect(testExtraManifestCM.GetLabels()).To(HaveKeyWithValue(backupLabel, backupLabelValue), "ConfigMap %s/%s missing backup label", testExtraManifestCM.Namespace, testExtraManifestCM.Name)
2317+
2318+
// Verify pull secret has backup label
2319+
pullSecretKey := types.NamespacedName{
2320+
Name: pullSecret.Name,
2321+
Namespace: pullSecret.Namespace,
2322+
}
2323+
testPullSecret := &corev1.Secret{}
2324+
Expect(c.Get(ctx, pullSecretKey, testPullSecret)).To(Succeed())
2325+
Expect(testPullSecret.GetLabels()).To(HaveKeyWithValue(backupLabel, backupLabelValue), "Secret %s/%s missing backup label", testPullSecret.Namespace, testPullSecret.Name)
2326+
})
2327+
22102328
Context("when the cluster identity secrets exist", func() {
22112329
createSecret := func(name string, data map[string][]byte) {
22122330
s := corev1.Secret{

‎internal/credentials/credentials.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,7 @@ func (r *Credentials) createOrUpdateClusterCredentialSecret(ctx context.Context,
197197
metav1.SetMetaDataLabel(&secret.ObjectMeta, "hive.openshift.io/secret-type", secretType)
198198
metav1.SetMetaDataLabel(&secret.ObjectMeta, SecretResourceLabel, SecretResourceValue)
199199
metav1.SetMetaDataLabel(&secret.ObjectMeta, secretPreservationLabel, secretPreservationValue)
200+
metav1.SetMetaDataLabel(&secret.ObjectMeta, "cluster.open-cluster-management.io/backup", "true")
200201

201202
// Update the Secret object with the desired data
202203
secret.Data = data

‎internal/credentials/credentials_test.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,17 @@ var _ = Describe("Credentials", func() {
269269
Expect(len(secret.OwnerReferences)).To(Equal(1))
270270
Expect(secret.OwnerReferences[0].Name).To(Equal(clusterDeployment.Name))
271271
})
272+
273+
It("sets the backup label", func() {
274+
name := "test-secret"
275+
data := map[string][]byte{"thing": []byte("stuff")}
276+
Expect(cm.createOrUpdateClusterCredentialSecret(ctx, log, clusterDeployment, name, data, "thing")).To(Succeed())
277+
278+
secret := corev1.Secret{}
279+
key := types.NamespacedName{Name: name, Namespace: clusterDeployment.Namespace}
280+
Expect(c.Get(ctx, key, &secret)).To(Succeed())
281+
Expect(secret.Labels).To(HaveKeyWithValue("cluster.open-cluster-management.io/backup", "true"))
282+
})
272283
})
273284

274285
createSecret := func(name string, data map[string][]byte) {

0 commit comments

Comments
 (0)