Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove provisioning paused annotation #6306

Closed
wants to merge 2 commits into from
Closed

Remove provisioning paused annotation #6306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b0e9caf
Remove provisioning paused annotation
carbonin May 10, 2024
5e534e3
Refactor to deduplicate some code between BMH and provisioning handling
carbonin May 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
77 changes: 51 additions & 26 deletions internal/ignition/ignition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,15 @@ import (
"github.com/openshift/assisted-service/pkg/mirrorregistries"
"github.com/openshift/assisted-service/pkg/s3wrapper"
"github.com/openshift/assisted-service/pkg/staticnetworkconfig"
metal3iov1alpha1 "github.com/openshift/cluster-baremetal-operator/api/v1alpha1"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"github.com/thoas/go-funk"
"github.com/vincent-petithory/dataurl"
"golang.org/x/sync/errgroup"
"gopkg.in/yaml.v2"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
k8sjson "k8s.io/apimachinery/pkg/runtime/serializer/json"
"k8s.io/client-go/kubernetes/scheme"
)
Expand Down Expand Up @@ -885,13 +887,18 @@ func (g *installerGenerator) updateBootstrap(ctx context.Context, bootstrapPath
// drop this from the list of Files because we don't want to run BMO
continue
}
if provErr := removeProvisioningPausedAnnotation(&config.Storage.Files[i]); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, whenever assisted is creating a BMH resource the BMH status is unmanaged (SaaS) or externally provisioned (ZTP). if assisted behavior change or ironic/metal3 will try to update the BMh status we will end up with the original issue (https://issues.redhat.com//browse/OCPBUGS-33157).
Perhaps removing this annotation from the assited-installer-controller after all control plane nodes are ready is preferable since it aligns with the desired CBO behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The annotation is purely to prevent BMO in the cluster interfering with the BMO on the baremetal bootstrap host while it is still doing its work.
In a system with no baremetal bootstrap host it is not required.

provErr = fmt.Errorf("failed to remove provisioning paused annotation: %w", provErr)
log.Error(provErr.Error())
return provErr
}
case isMOTD(&config.Storage.Files[i]):
// workaround for https://github.com/openshift/machine-config-operator/issues/2086
g.fixMOTDFile(&config.Storage.Files[i])
case isBMHFile(&config.Storage.Files[i]):
// extract bmh
bmh, err2 := fileToBMH(&config.Storage.Files[i]) //nolint,shadow
if err2 != nil {
bmh := &bmh_v1alpha1.BareMetalHost{}
if err2 := deserializeToObject(&config.Storage.Files[i], bmh); err2 != nil {
log.Errorf("error parsing File contents to BareMetalHost: %v", err2)
return err2
}
Expand Down Expand Up @@ -963,23 +970,55 @@ func isBaremetalProvisioningConfig(file *config_latest_types.File) bool {
return strings.Contains(file.Node.Path, "baremetal-provisioning-config")
}

func fileToBMH(file *config_latest_types.File) (*bmh_v1alpha1.BareMetalHost, error) {
func removeProvisioningPausedAnnotation(file *config_latest_types.File) error {
provisioning := &metal3iov1alpha1.Provisioning{}
if err := deserializeToObject(file, provisioning); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause us problems in future, because the API definition will have to be kept up to date every time a field is added to the Provisioning resource, and we will certainly forget that this is here and then wonder why the new data is getting silently dropped.

This might still be the best thing to do in the short term, but it's not a great substitute for a long-term fix that doesn't require adding this annotation at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How long do you think it will be until we get a proper long-term fix?
Do you think doing this in the controller against the actual cluster with a patch API call will be any more maintainable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We got a successful test on openshift/installer#8391 but there's a few moving parts so it might take a minute to land.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a patch API call would avoid the out-of-date-API-definition problem.

return err
}
delete(provisioning.Annotations, "provisioning.metal3.io/paused")

source, err := encodedYAMLFileSource(provisioning)
if err != nil {
return fmt.Errorf("failed to create file source for provisioning resource: %w", err)
}
file.Contents.Source = &source

return nil
}

func deserializeToObject(file *config_latest_types.File, o runtime.Object) error {
parts := strings.Split(*file.Contents.Source, "base64,")
if len(parts) != 2 {
return nil, errors.Errorf("could not parse source for file %s", file.Node.Path)
return fmt.Errorf("could not parse source for file %s", file.Node.Path)
}
decoded, err := base64.StdEncoding.DecodeString(parts[1])
if err != nil {
return nil, err
return fmt.Errorf("failed to decode file: %w", err)
}

bmh := &bmh_v1alpha1.BareMetalHost{}
_, _, err = scheme.Codecs.UniversalDeserializer().Decode(decoded, nil, bmh)
_, _, err = scheme.Codecs.UniversalDeserializer().Decode(decoded, nil, o)
if err != nil {
return nil, err
return fmt.Errorf("failed to deserialize object: %w", err)
}
return nil
}

return bmh, nil
func encodedYAMLFileSource(o runtime.Object) (string, error) {
serializer := k8sjson.NewSerializerWithOptions(
k8sjson.DefaultMetaFactory, nil, nil,
k8sjson.SerializerOptions{
Yaml: true,
Pretty: true,
Strict: true,
},
)
buf := bytes.Buffer{}
if err := serializer.Encode(o, &buf); err != nil {
return "", err
}

// remove status if exists
res := bytes.Split(buf.Bytes(), []byte("status:\n"))
return "data:text/plain;charset=utf-8;base64," + base64.StdEncoding.EncodeToString(res[0]), nil
}

// fixMOTDFile is a workaround for a bug in machine-config-operator, where it
Expand Down Expand Up @@ -1076,24 +1115,10 @@ func (g *installerGenerator) modifyBMHFile(file *config_latest_types.File, bmh *
bmh.Spec.ExternallyProvisioned = true
}

serializer := k8sjson.NewSerializerWithOptions(
k8sjson.DefaultMetaFactory, nil, nil,
k8sjson.SerializerOptions{
Yaml: true,
Pretty: true,
Strict: true,
},
)
buf := bytes.Buffer{}
err = serializer.Encode(bmh, &buf)
source, err := encodedYAMLFileSource(bmh)
if err != nil {
return err
return fmt.Errorf("failed to create file source for BMH resource: %w", err)
}

// remove status if exists
res := bytes.Split(buf.Bytes(), []byte("status:\n"))
encodedBMH := base64.StdEncoding.EncodeToString(res[0])
source := "data:text/plain;charset=utf-8;base64," + encodedBMH
file.Contents.Source = &source

return nil
Expand Down
74 changes: 52 additions & 22 deletions internal/ignition/ignition_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,12 @@ import (
"github.com/openshift/assisted-service/pkg/mirrorregistries"
"github.com/openshift/assisted-service/pkg/s3wrapper"
"github.com/openshift/assisted-service/pkg/staticnetworkconfig"
metal3iov1alpha1 "github.com/openshift/cluster-baremetal-operator/api/v1alpha1"
"github.com/sirupsen/logrus"
"github.com/vincent-petithory/dataurl"
"gopkg.in/yaml.v2"
"gorm.io/gorm"
"k8s.io/client-go/kubernetes/scheme"
)

var (
Expand Down Expand Up @@ -108,7 +110,19 @@ var _ = Describe("Bootstrap Ignition Update", func() {
"verification": {}
},
"mode": 420
}
},
{
"overwrite": true,
"path": "/opt/openshift/openshift/99_baremetal-provisioning-config.yaml",
"user": {
"name": "root"
},
"contents": {
"source": "data:text/plain;charset=utf-8;base64,YXBpVmVyc2lvbjogbWV0YWwzLmlvL3YxYWxwaGExCmtpbmQ6IFByb3Zpc2lvbmluZwptZXRhZGF0YToKICBuYW1lOiBwcm92aXNpb25pbmctY29uZmlndXJhdGlvbgogIGFubm90YXRpb25zOgogICAgIyBXZSB3YW50IHRvIHBhdXNlIHRoZSBtZXRhbDMgcHJvdmlzaW9uaW5nIHNlcnZpY2VzIHVudGlsIHdlIGNhbiBiZSBzdXJlCiAgICAjIHRoYXQgYWxsIGNvbnRyb2wgcGxhbmUgbm9kZXMgYXJlIHJlYWR5LgogICAgcHJvdmlzaW9uaW5nLm1ldGFsMy5pby9wYXVzZWQ6ICJ0cnVlIgpzcGVjOgogIHByb3Zpc2lvbmluZ0ludGVyZmFjZTogIiIKICBwcm92aXNpb25pbmdJUDogIiIKICBwcm92aXNpb25pbmdOZXR3b3JrQ0lEUjogIiIKICBwcm92aXNpb25pbmdOZXR3b3JrOiAiRGlzYWJsZWQiCiAgcHJvdmlzaW9uaW5nREhDUFJhbmdlOiAiIgogIHByb3Zpc2lvbmluZ09TRG93bmxvYWRVUkw6ICIiCiAgd2F0Y2hBbGxOYW1lc3BhY2VzOiBmYWxzZQo=",
"verification": {}
},
"mode": 420
}
]
}
}`
Expand Down Expand Up @@ -164,7 +178,8 @@ var _ = Describe("Bootstrap Ignition Update", func() {
foundNMConfig = true
}
}
bmh, _ = fileToBMH(file)
bmh = &bmh_v1alpha1.BareMetalHost{}
Expect(deserializeToObject(file, bmh)).To(Succeed())
Expect(foundNMConfig).To(BeTrue(), "file /etc/NetworkManager/conf.d/99-kni.conf not present in bootstrap.ign")
})

Expand All @@ -188,8 +203,8 @@ var _ = Describe("Bootstrap Ignition Update", func() {
Expect(err).ToNot(HaveOccurred())
for i := range config.Storage.Files {
if isBMHFile(&config.Storage.Files[i]) {
bmhFile, err2 := fileToBMH(&config.Storage.Files[i]) //nolint,shadow
Expect(err2).ToNot(HaveOccurred())
bmhFile := &bmh_v1alpha1.BareMetalHost{}
Expect(deserializeToObject(&config.Storage.Files[i], bmhFile)).To(Succeed())
Expect(bmhIsMaster(bmhFile, masterHostnames, workerHostnames)).To(Equal(masterExpected))
return
}
Expand All @@ -208,23 +223,38 @@ var _ = Describe("Bootstrap Ignition Update", func() {
})

Describe("update bootstrap.ign", func() {
Context("with 1 master", func() {
It("got a tmp workDir", func() {
Expect(workDir).NotTo(Equal(""))
})
It("adds annotation", func() {
Expect(err).NotTo(HaveOccurred())
Expect(bmh.Annotations).To(HaveKey(bmh_v1alpha1.StatusAnnotation))
})
It("adds the marker file", func() {
var found bool
for _, f := range config.Storage.Files {
if f.Path == "/opt/openshift/assisted-install-bootstrap" {
found = true
}
It("got a tmp workDir", func() {
Expect(workDir).NotTo(Equal(""))
})
It("adds annotation", func() {
Expect(err).NotTo(HaveOccurred())
Expect(bmh.Annotations).To(HaveKey(bmh_v1alpha1.StatusAnnotation))
})
It("adds the marker file", func() {
var found bool
for _, f := range config.Storage.Files {
if f.Path == "/opt/openshift/assisted-install-bootstrap" {
found = true
}
Expect(found).To(BeTrue())
})
}
Expect(found).To(BeTrue())
})
It("removes the provisioning CR paused annotation", func() {
var provisioning *metal3iov1alpha1.Provisioning
for _, f := range config.Storage.Files {
if strings.Contains(f.Node.Path, "baremetal-provisioning-config") {
parts := strings.Split(*f.Contents.Source, "base64,")
Expect(len(parts)).To(Equal(2))
decoded, err := base64.StdEncoding.DecodeString(parts[1])
Expect(err).NotTo(HaveOccurred())
provisioning = &metal3iov1alpha1.Provisioning{}
_, _, err = scheme.Codecs.UniversalDeserializer().Decode(decoded, nil, provisioning)
Expect(err).NotTo(HaveOccurred())
break
}
}
Expect(provisioning).NotTo(BeNil())
Expect(provisioning.Annotations).ToNot(HaveKey("provisioning.metal3.io/paused"))
})
})
})
Expand Down Expand Up @@ -2465,8 +2495,8 @@ var _ = Describe("Bare metal host generation", func() {
outputFile := &config_32_types.File{}
err = generator.modifyBMHFile(outputFile, inputObject, host)
Expect(err).ToNot(HaveOccurred())
outputObject, err := fileToBMH(outputFile)
Expect(err).ToNot(HaveOccurred())
outputObject := &bmh_v1alpha1.BareMetalHost{}
Expect(deserializeToObject(outputFile, outputObject)).To(Succeed())

// Extract the content of the status annotation:
Expect(outputObject.Annotations).To(HaveKey(bmh_v1alpha1.StatusAnnotation))
Expand Down