MGMT-17365: Add support for setting agent labels in BMH as annotations #6124
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ori-amizur: This pull request references MGMT-17365 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.16.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
size/L
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6124 +/- ##
==========================================
+ Coverage 68.31% 68.75% +0.43%
==========================================
Files 242 243 +1
Lines 35817 37357 +1540
==========================================
+ Hits 24470 25684 +1214
- Misses 9221 9454 +233
- Partials 2126 2219 +93
|
|
/retest |
|
/test edge-e2e-ai-operator-disconnected-capi |
There was a problem hiding this comment.
I don't particularly like the json API and the imperitive nature of the annotation values.
This is targeted toward users of ZTP and I don't think those users are going to be tweaking these annotations to add and remove things after initial deployment.
I'm reminded of @mhrivnak's reasoning in #5893 (comment) and rereading that comment in the scope of this PR makes me think we should stick with a simpler solution:
If the annotation exists on the BMH, the corresponding label should exist on the agent.
And if the annotation doesn't exist in BMH, should it be deleted from the Agent ? |
|
Please find an appropriate location inside this repository to document this feature before merging it. |
I 100% agree. So what's the plan then? I dont think this is an acceptable solution for a supported code |
Not sure what the "this" is here, but my idea was that adding an annotation on the BMH would push that as a label to the agent and removing it would do nothing. The justification is that users adding things on the BMH are not doing this as an interactive process so simply replicating over should be enough. Another option would be to enfoce a naming scheme for the label on the agent that would allow our controller (BMAC) to know if a label on the agent was added from the BMH, but is also not in the annotations in the BMH currently (and thus should be removed). This should be okay because nothing external cares about the agent label name (as opposed to node labels). |
|
Since deletion is not required, the annotation value will be identical agent label value. |
|
@ori-amizur: This pull request references MGMT-17365 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.16.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Can you update the PR description to match the current behavior?
Please find an appropriate location inside this repository to document this feature before merging it.
Specifically this should be added to the existing list of BMH annotation info in https://github.com/openshift/assisted-service/tree/master/docs/hive-integration#bare-metal-operator-integration
BMH is the custom resource that is used to add a new host. Agent on the other hand is created automatically when a host registers. Since there is a need to control agent labels the following agent label support was added: In order to add an entry that controls agent label, a new BMH annotation needs to be added. The annotation key is prefixed with the string 'bmac.agent-install.openshift.io.agent-label.'. The remainder of the annotation is considered the label key. The value of the annotation is identical to the agent label value. Note: agent labels cannot be deleted by usage of BMH annotations.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin, ori-amizur 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 |
|
/retest |
1 similar comment
|
/retest |
|
/hold Revision 32623ef was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@ori-amizur: all tests passed! 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. |
BMH is the custom resource that is used to add a new host. Agent on the other hand is created automatically when a host registers. Since there is a need to control agent labels the following agent label support was added:
In order to add an entry that controls agent label, a new BMH annotation needs to be added.
The annotation key is prefixed with the string
'bmac.agent-install.openshift.io.agent-label.'. The remainder of the annotation is considered the label key.
The value of the annotation is a JSON dictionary with 2 possible keys. The key 'operation' can contain one of the values ["add","delete"] which mean that the label can either added , or deleted. The dictionary key 'value' contains the label value.
The value of the annotation is identical to the agent label value.
Note: agent labels cannot be deleted by usage of BMH annotations.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
/cc @carbonin