Bug 1840782: Update conditions under which metal3 pod would be deployed #560
Conversation
|
@hardys and @yevgeny-shnaidman could this be a way to handle the other baremetal mode that you were discussing here https://github.com/openshift/enhancements/pull/212/files#r409393934 |
|
Yes, it might.
One suggestion: if it is not too late, maybe you can move the call to
isBaremetalProvisioningConfigAvailable
from syncAll to maoConfigFromInfrastructure, to where the value of
usingBareMetal is set. It can be something like:
usingBareMetal := (provider == osconfigv1.BareMetalPlatformType &&
isBaremetalProvisioningConfigAvailable())
then you don't need to change syncAll and syncBaremetalControllers functions
at all.
…On Fri, Apr 17, 2020 at 11:35 PM Sandhya Dasu ***@***.***> wrote:
@hardys <https://github.com/hardys> and @yevgeny-shnaidman
<https://github.com/yevgeny-shnaidman> could this be a way to handle the
other baremetal mode that you were discussing here
https://github.com/openshift/enhancements/pull/212/files#r409393934
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#560 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOPNM5KY7KSDUBVBFVRZQWTRNC4QPANCNFSM4MLAVGTQ>
.
|
|
i am OK with this approach. Regarding installer change - since this is a
temporary solution until BMO becomes full-fledge operator, we will be able
to remove the provisioning CR from bootstrap ignition after the installer
has created it, so we won't need any change in the installer code
…On Tue, Apr 21, 2020 at 7:36 PM Sandhya Dasu ***@***.***> wrote:
Yes, it might. One suggestion: if it is not too late, maybe you can move
the call to isBaremetalProvisioningConfigAvailable from syncAll to
maoConfigFromInfrastructure, to where the value of usingBareMetal is set.
It can be something like: usingBareMetal := (provider ==
osconfigv1.BareMetalPlatformType &&
isBaremetalProvisioningConfigAvailable()) then you don't need to change
syncAll and syncBaremetalControllers functions at all.
@yevgeny-shnaidman <https://github.com/yevgeny-shnaidman> yes, we can
improve the implementation. I want confirmation that you are Ok with the
approach that I am suggesting here i.e not creating the provisioning CR
when we do not want metal3 to be deployed. If that is OK, then we need a
corresponding change in the installer to not create the provisioning CR for
this other baremetal mode (UPI ?). There we will need a configuration flag
to distinguish between the 2 baremetal installs. cc @hardys
<https://github.com/hardys> @stbenjam <https://github.com/stbenjam> @imain
<https://github.com/imain>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#560 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOPNM5PEJOLHTIGUBIZGJ43RNXDDLANCNFSM4MLAVGTQ>
.
|
@yevgeny-shnaidman I would like to keep the check for the presence of the provisioning CR within the sync logic because it is possible that we are unable to read the CR due to some transient connectivity issues. We want to keep retrying to read that. With the current implementation we will not be flooding the logs with info messages when the provisioning CR is not really present. |
|
/test e2e-aws-scaleup-rhel7 |
|
/test e2e-aws-upgrade |
|
/test e2e-aws-operator |
|
/test e2e-azure-operator |
|
Would like to point out that this fix is not temporary. This same logic would be carried forward to the new operator when it is ready. |
|
/test e2e-aws-operator |
|
/test e2e-azure-operator |
|
that's ok, because eventually we would like to use baremetal operator, only
with our provisioner. Not using it at all is for the intermediate stage of
our product
…On Wed, Apr 22, 2020 at 6:31 PM Sandhya Dasu ***@***.***> wrote:
Would like to point out that this fix is not temporary. This same logic
would be carried forward to the new operator when it is ready.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#560 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOPNM5MQBTFIKMLNW6FCMA3RN4EVJANCNFSM4MLAVGTQ>
.
|
sadasu
changed the title
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
sadasu
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
@sadasu any eta on this commit? |
|
@enxebre and @JoelSpeed could you please take a look? |
|
/test e2e-aws-scaleup-rhel7 |
|
/test e2e-azure-operator |
|
/retest |
|
/test e2e-azure |
|
/test e2e-gcp |
|
/test e2e-azure |
|
/lgtm |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/test e2e-azure |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elmiko The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
8 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@sadasu: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@sadasu: All pull requests linked via external trackers have merged: Bugzilla bug 1840782 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Changes to accommodate Baremetal UPI deployments or assisted baremetal deployments where the platform type would be Baremetal without a need to deploy the metal3 pod.