OSASINFRA-4329: Ensure tag extension is available#421
Conversation
[release-0.9] 🌱 Move webhooks into pkg/webhooks
Delete the OpenStack instance before deleting its network ports. This prevents "Policy doesn't allow compute:detach primary port" errors that occur when attempting to delete a primary NIC while the instance still exists. Deleting the instance automatically detaches all interfaces, so the explicit detach call is no longer needed.
…ace-deletion-order-0.9 [release-0.9] Fix instance deletion order
Before setting tags on ports or trunks, check whether the neutron standard-attr-tag extension is available. If the extension is absent: - Return an error if port-specific tags were explicitly requested - Skip tagging if instance tags are present, logging a warning Adds getStdAttrTagSupport() helper to list neutron extensions and check for the standard-attr-tag alias, with unit tests covering both supported and unsupported extension scenarios. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dan Childers <dchilder@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@danchild: This pull request references OSASINFRA-4329 which is a valid jira issue. 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. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/assign @stephenfin |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| tagsAreSupported, err := s.getStdAttrTagSupport() | ||
|
|
||
| if err != nil { | ||
| record.Warnf(eventObject, "FailedGetTagSupport", "Failed to verify neutron support for the standard-attr-tag extension, skipping tags: %v", err) | ||
| } | ||
|
|
||
| // Return error if user sets port tags; Skip over but warn for instance tags | ||
| if !tagsAreSupported { | ||
| if len(portOpts.Tags) > 0 { | ||
| err = fmt.Errorf("cannot tag port %s using port tags; tags are configured but neutron does not support the standard-attr-tag extension", portName) | ||
| record.Warnf(eventObject, "FailedTagPort", "Failed to tag port %s: %v", portName, err) | ||
| return nil, err | ||
| } | ||
|
|
||
| record.Warnf(eventObject, "FailedTagPort", "Neutron does not support the standard-attr-tag extension, skipping instance tags for port %s", portName) | ||
| } |
There was a problem hiding this comment.
Can we place all of this inside an if len(tags) > 0 block? I don't see any point making a request to the server if we don't have any tags to update. Something like
tagsAreSupported := False
if len(tags) > 0 {
// ... your changes
}
if len(tags) > 0 && tagsAreSupported
5 participants
Check for standard-attr-tag extension before tagging ports
Upstream CAPO
release-0.9permits users to set tags on instances which eventually propagates down to ports. However, neutron'sstandard-attr-tagextension is not supported by all OpenStack versions. The behavior prevents port creation which prevents instances from being created as well.This patch prevents these errors by doing the following:
standard-attr-tagsupportThis change is made against downstream CAPO rather than the upstream, because MAPO has CAPO release-0.9 as a hard dependency which is reaching EOL. Upstream CAPO has diverged substantially since that release, making a direct upstream backport impractical for MAPO consumers at this time. If accepted, MAPO will need to add a
replace directivein its go.mod to redirect CAPO from upstream to downstream.It is assumed that the same defect exists in the upstream CAPO, and an investigation will take place to confirm the suspicion. If true, the same defect will be addressed there to prevent regressions down the path of a full CAPO replacement of MAPO.