Repository navigation
MGMT-17478: Use ICC config secret for converged flow info when available - #6533
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@adriengentil: This pull request references MGMT-17478 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.17.0" version, but no target version was set. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
size/L
|
/test ? |
|
@adriengentil: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
DetailsIn 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-sigs/prow repository. |
|
@adriengentil: This pull request references MGMT-17478 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.17.0" version, but no target version was set. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/test edge-lint edge-unit-test edge-e2e-ai-operator-ztp |
|
@carbonin can you have a first look? 🙏 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6533 +/- ##
==========================================
+ Coverage 68.42% 68.51% +0.08%
==========================================
Files 247 247
Lines 36425 36599 +174
==========================================
+ Hits 24924 25075 +151
- Misses 9293 9310 +17
- Partials 2208 2214 +6
|
32c5a77 to
2d78319
Compare
|
/test edge-lint edge-unit-test edge-e2e-ai-operator-ztp |
When available, read the ICC secret set by BMO in order to retrieve the agent image, the agent URL(s), and the inspector URL(s). This this change, we wil try to: 1/ Get the agent image from the user override 2/ if 1/ is not found, get the image from the ICC configuration 3/ if 2/ is not found, get the image from the HUB cluster 4/ if 3/ is not found, return the default image
fc549af to
3eb740e
Compare
|
/test edge-lint edge-unit-test |
| } | ||
| } | ||
|
|
||
| func (r *PreprovisioningImageReconciler) getIronicConfig(ctx context.Context, log logrus.FieldLogger, infraEnv *aiv1beta1.InfraEnv, infraEnvInternal *common.InfraEnv) (*ICCConfig, error) { |
There was a problem hiding this comment.
No change needed, I'm just really happy with how much easier this is to read now. 🎉
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| return ironicIPs, inspectorIPs, nil | ||
| } | ||
|
|
||
| func (r *bmoUtils) GetICCConfig(ctx context.Context) (*ICCConfig, error) { |
There was a problem hiding this comment.
This doesn't need to be exported if we're only using it from this package.
There was a problem hiding this comment.
I changed it, not sure if having a private method in BMOUtils interface is very idiomatic, but it looks like it's mainly here to get a mock out of it. Let me know if we should do differently 🤔
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriengentil, 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 |
|
/retest |
|
/retest |
|
/retest |
|
@adriengentil: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
|
||
| ironicInspectorBaseURL, ok := secret.Data[ironicInspectorBaseURLKey] | ||
| if !ok { | ||
| return nil, fmt.Errorf(configKeyNotFoundError, ironicInspectorBaseURLKey, secret.Name, secret.Namespace) |
There was a problem hiding this comment.
Late to this party, sorry. This is not quite correct: the inspector URL can and actually will be missing.
There was a problem hiding this comment.
In that case, how we get the inspector URL? From the preprovisioning info? Or derived from the ironicBaseURL ?
There was a problem hiding this comment.
Ok, if understand well, we can leave the inspector URL empty, and it will be auto-magically filled using the ICC lib openshift/image-customization-controller@26ce5d6
There was a problem hiding this comment.
😕 but we don't use any of the icc stuff, right?
There was a problem hiding this comment.
we use it here:
…ble (openshift#6533) * MGMT-17478: Use ICC config secret for converged flow info when available When available, read the ICC secret set by BMO in order to retrieve the agent image, the agent URL(s), and the inspector URL(s). This this change, we wil try to: 1/ Get the agent image from the user override 2/ if 1/ is not found, get the image from the ICC configuration 3/ if 2/ is not found, get the image from the HUB cluster 4/ if 3/ is not found, return the default image * Add context to GetICCConfig method * remove space * refactor tiered logic * Create GetImageArchitecture mehod * fix lint * refactor logs and getIronicAgentImageByRelease * add infraenv in logger earlier * make getICCConfig private
* MGMT-17478: Use ICC config secret for converged flow info when available (#6533) * MGMT-17478: Use ICC config secret for converged flow info when available When available, read the ICC secret set by BMO in order to retrieve the agent image, the agent URL(s), and the inspector URL(s). This this change, we wil try to: 1/ Get the agent image from the user override 2/ if 1/ is not found, get the image from the ICC configuration 3/ if 2/ is not found, get the image from the HUB cluster 4/ if 3/ is not found, return the default image * Add context to GetICCConfig method * remove space * refactor tiered logic * Create GetImageArchitecture mehod * fix lint * refactor logs and getIronicAgentImageByRelease * add infraenv in logger earlier * make getICCConfig private * MGMT-18505: Fix installation from a 4.17 hub with converged flow (#6639) * This allows ironic inspector URL to be missing in ICC config secret. In 4.17 this is expected to be missing as the inspector service as been removed. If this URL is provided the agent will attempt to contact the inspector service when it shouldn't causing the install to fail. In earlier versions the secret will not be present so the controller will continue to provide the inspector service URL as before. Resolves https://issues.redhat.com/browse/OCPBUGS-37472 * Update image-customization-controller to release-4.16 branch This includes a patch which removes the default for the inspector URL. This is required because when deploying from a 4.17 hub the inspector URL will not be present in the information on the cluster and we don't want that URL to be set in the ignition. --------- Co-authored-by: Adrien Gentil <agentil@redhat.com>
…n available (openshift#6533)" This reverts commit 88b5e51.
4 participants
When available, read the ICC secret set by BMO in order to retrieve the
agent image, the agent URL(s), and the inspector URL(s).
This this change, we wil try to:
1/ Get the agent image from the user override
2/ if 1/ is not found, get the image from the ICC configuration
3/ if 2/ is not found, get the image from the HUB cluster
4/ if 3/ is not found, return the default image