-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1824426: tag primary OpenStack networks #3483
Conversation
@Fedosin: This pull request references Bugzilla bug 1824426, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
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. |
@Fedosin: This pull request references Bugzilla bug 1824426, which is valid. 3 validation(s) were run on this bug
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. |
If we tag a custom network / aka IF machinesNetwork != nil |
/hold |
@iamemilio done :) |
/hold cancel |
@Fedosin: An error was encountered adding this pull request to the external tracker bugs for bug 1824426 on the Bugzilla server at https://bugzilla.redhat.com:
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. |
/hold cancel |
pkg/destroy/openstack/openstack.go
Outdated
return untagNetwork(opts, networkTag, logger) | ||
}) | ||
if err != nil { | ||
return errors.Errorf("Unrecoverable error/timed out: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is "timed out" the only possible error here. If it is not it could make it difficult to debug. the user could be looking for a time out, when that is not the problem. 'err' sould have enough info in its message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, there could also be a bug with multiple networks with the same tag... I separated them as you ask.
pkg/destroy/openstack/openstack.go
Outdated
|
||
// untagNetworks removes a tag from the network | ||
func untagNetwork(opts *clientconfig.ClientOpts, networkTag string, logger logrus.FieldLogger) (bool, error) { | ||
logger.Debug("Untagging openstack networks") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add network tagg to the message . Useful for user to verify which tags should have been removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkg/destroy/openstack/openstack.go
Outdated
} | ||
|
||
if len(allNetworks) > 1 { | ||
return false, errors.Errorf("more than one network with tag %v", networkTag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
caps on "More..." Every other message starts with caps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing but nits on strings.
/test e2e-aws |
pkg/destroy/openstack/openstack.go
Outdated
@@ -110,6 +115,12 @@ func (o *ClusterUninstaller) Run() error { | |||
} | |||
} | |||
|
|||
// we need to untag the custom network if it was provided by the user | |||
err := untagRunner(opts, o.InfraID+"-primaryClusterNetwork", o.Logger) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a general comment, untagRunner
should be generic and not take the networkTag as an argument. Instead it should be the infra-id, and we derive the network-tag from it when calling untagNetwork
from untagRunner
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, makes sense
} | ||
|
||
if len(allNetworks) > 1 { | ||
return false, errors.Errorf("More than one network with tag %v", networkTag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why make it an error? That will cause the untagRunner to enter a loop to perform an action it can't do.
IMO, as opposed to deleting resources when we're not sure we were the ones creating them, it's OK to remove (internally used) tags from resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we return an error, then untagRunner stops working immediately, no matter how many attempts there are left. So, for the runner it basically means - stop looping and return the error.
https://github.com/kubernetes/apimachinery/blob/release-1.17/pkg/util/wait/wait.go#L293
Despite the fact we consider the tag to be unique because it contains an infra ID, users can still add the tag to other networks. So removing them from all networks can lead to unpleasant side effects, which means we must stop the execution and do not try to repeat the untagging.
In the case of multiple added networks the tag <infraID>-primaryClusterNetwork should allow cluster-api-provider-openstack to define which IP address to set as the primary one for machines. Now CAPO can't do this, because Neutron returns the list of networks in alphabetical order.
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: iamemilio 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 Please review the full test history for this PR and help us cut down flakes. |
4 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. |
@Fedosin: Some pull requests linked via external trackers have merged: . The following pull requests linked via external trackers have not merged:
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. |
/cherry-pick release-4.4 |
@adduarte: #3483 failed to apply on top of branch "release-4.4":
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. |
In the case of multiple added networks the tag
<infraID>-primaryClusterNetwork
should allow cluster-api-provider-openstack to define which IP address to set as the primary one for machines.Now CAPO can't do this, because Neutron returns the list of networks in alphabetical order.