Skip to content

MGMT-2888 Add marker file to bootstrap ignition #672

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

Merged
openshift-merge-robot merged 1 commit into from
Nov 16, 2020
Merged

MGMT-2888 Add marker file to bootstrap ignition #672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions internal/ignition/ignition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,8 @@ func (g *installerGenerator) updateBootstrap(bootstrapPath string) error {

config.Storage.Files = newFiles

setFileInIgnition(config, "/opt/openshift/assisted-install-bootstrap", "data:,", false, 420)

err = writeIgnitionFile(bootstrapPath, config)
if err != nil {
g.log.Error(err)
Expand Down
22 changes: 18 additions & 4 deletions internal/ignition/ignition_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ var _ = Describe("Bootstrap Ignition Update", func() {
var (
err error
examplePath string
file *config_31_types.File
bmh *bmh_v1alpha1.BareMetalHost
config config_31_types.Config
)

BeforeEach(func() {
Expand All @@ -103,10 +103,15 @@ var _ = Describe("Bootstrap Ignition Update", func() {
err = g.updateBootstrap(examplePath)

bootstrapBytes, _ := ioutil.ReadFile(examplePath)
config, _, err1 := config_31.Parse(bootstrapBytes)
config, _, err1 = config_31.Parse(bootstrapBytes)
Expect(err1).NotTo(HaveOccurred())
Expect(config.Storage.Files).To(HaveLen(1))
file = &config.Storage.Files[0]

var file *config_31_types.File
for i := range config.Storage.Files {
if isBMHFile(&config.Storage.Files[i]) {
file = &config.Storage.Files[i]
}
}
bmh, _ = fileToBMH(file)
})

Expand All @@ -119,6 +124,15 @@ var _ = Describe("Bootstrap Ignition Update", 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" {
Copy link

@hexfusion hexfusion Nov 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

the current pattern is to use a file extension of done to indicate your component has completed its tasks on the bootstrap node. I would prefer if we maintained that.

https://github.com/openshift/installer/blob/ba6d7fe087eada5a4fc260064c85a484a8c45aaf/data/data/bootstrap/files/usr/local/bin/bootkube.sh.template#L287

Copy link
Member Author

@carbonin carbonin Nov 13, 2020

Choose a reason for hiding this comment

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

Right, but that timing doesn't make sense for what we're using this for. This is exactly why I suggested not using the .done extension.

We need this file to be here before we've completed our work on the bootstrap. As an installer I would consider "our work" to be the entire bootstrap node lifecycle.

Copy link

@hexfusion hexfusion Nov 13, 2020

Choose a reason for hiding this comment

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

technically done is a reference to the rendering of manifests by operator render commands via bootkube. your component does not perform those actions to my knowledge.

We need this file to be here before we've completed our work on the bootstrap. As an installer I would consider "our work" to be the entire bootstrap node lifecycle.

That may be true but that is not the meaning of the file's existence. Each operator adds this file so that simple checks can exist to omit rerun of logic on systemd unit retry. We are just piggybacking the proccess and an observable condition.

Copy link

@hexfusion hexfusion Nov 13, 2020

Choose a reason for hiding this comment

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

basically, why create a new pattern?

Copy link
Member Author

@carbonin carbonin Nov 13, 2020

Choose a reason for hiding this comment

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

basically, why create a new pattern?

Because the file we're creating isn't serving the same purpose as it is in all the other cases. I think it's more confusing to conform to an existing pattern if none of the semantics about that pattern apply to our case.

If there's a better location or name for this file that would be more appropriate for what we're doing I'd be happy to move it.

Copy link

@hexfusion hexfusion Nov 13, 2020

Choose a reason for hiding this comment

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

Your component is performing a no-op and thus is done in the context provided. This seems straightforward to me.. we will read whatever you write to disk I gave my reasoning.

Copy link
Contributor

@eranco74 eranco74 Nov 15, 2020

Choose a reason for hiding this comment

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

Don't know how this is piggybacking on the installer ".done" pattern.
Except for both use a file.
There is no operator, there is no rendering, and the file purpose is different.
Seems that it's targeting etcd on the bootstrap node, nothing else.

found = true
}
}
Expect(found).To(BeTrue())
})
})
})
})
Expand Down