MGMT-2888 Add marker file to bootstrap ignition #672
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
carbonin
force-pushed
the
add_bootstrap_marker_file
branch
from
4de159e to
a4c0a0c
Compare
|
I don't have any strong opinions about the name. This works for me. Thanks! |
internal/ignition/ignition_test.go
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
basically, why create a new pattern?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Let's try this in conjunction with openshift/cluster-etcd-operator#449 |
This adds a marker file to the bootstrap ignition so that the rest of the openshift installation knows that this cluster is using the assisted-installer and will need, for some time, to have a healthy 2-member etcd cluster. This is intended to replace the unsupported flag we currently use to allow a 2-member etcd cluster.
carbonin
force-pushed
the
add_bootstrap_marker_file
branch
from
a4c0a0c to
e78926e
Compare
Seems to have worked in my test 👍 |
|
@oshercc this seems like an infra issue: |
|
/lgtm |
Going forward we will use a marker file written to the bootstrap node. This is implemented in the service in openshift/assisted-service#672 and the etcd operator in openshift/cluster-etcd-operator#449 We also need to consider what version of openshift will contain the etcd operator enhancement we require before merging this. Fixes MGMT-2454
Going forward we will use a marker file written to the bootstrap node. This is implemented in the service in openshift/assisted-service#672 and the etcd operator in openshift/cluster-etcd-operator#449 Fixes MGMT-2454
Going forward we will use a marker file written to the bootstrap node. This is implemented in the service in openshift/assisted-service#672 and the etcd operator in openshift/cluster-etcd-operator#449 Fixes MGMT-2454
Going forward we will use a marker file written to the bootstrap node. This is implemented in the service in openshift/assisted-service#672 and the etcd operator in openshift/cluster-etcd-operator#449 Fixes MGMT-2454
Going forward we will use a marker file written to the bootstrap node. This is implemented in the service in openshift/assisted-service#672 and the etcd operator in openshift/cluster-etcd-operator#449 Fixes MGMT-2454
Going forward we will use a marker file written to the bootstrap node. This is implemented in the service in openshift/assisted-service#672 and the etcd operator in openshift/cluster-etcd-operator#449 Fixes MGMT-2454
Bumps the go-dependencies group with 2 updates: [github.com/onsi/gomega](https://github.com/onsi/gomega) and [golang.org/x/sys](https://github.com/golang/sys). Updates `github.com/onsi/gomega` from 1.31.1 to 1.32.0 - [Release notes](https://github.com/onsi/gomega/releases) - [Changelog](https://github.com/onsi/gomega/blob/master/CHANGELOG.md) - [Commits](onsi/gomega@v1.31.1...v1.32.0) Updates `golang.org/x/sys` from 0.17.0 to 0.18.0 - [Commits](golang/sys@v0.17.0...v0.18.0) --- updated-dependencies: - dependency-name: github.com/onsi/gomega dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: golang.org/x/sys dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
6 participants
This adds a marker file to the bootstrap ignition so that the rest of the openshift installation knows that this cluster is using the assisted-installer and will need, for some time, to have a healthy 2-member etcd cluster.
This is intended to replace the unsupported flag we currently use to allow a 2-member etcd cluster.
I thought the
.donenaming was a bit misleading as we're not done with bootstrap when we write this file.If anyone has a better name I'm open to suggestions.
cc @ironcladlou @hexfusion