-
Notifications
You must be signed in to change notification settings - Fork 595
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
[occm] feat : add load balancer listener tag using service annotation #2439
Conversation
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
Hi @KingDaemonX. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
@pierreprinetti can you take a look at this ?? |
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.
thanks for the PR. Is it still WIP? If so, please add documentation and tests and mark this PR as WIP. See also some major findings
/ok-to-test |
Also: please don't forget to add tests! At a minimum, a unit test to exercise tag splitting (with various configurations of spaces in it) would probably be a good idea |
do you mean the comments on the issue ?? |
97a664a
to
8cf9de8
Compare
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@pierreprinetti @kayrus please re-review i am working currently working on the test case, hence the WIP |
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.
This still needs a bunch of improvements.
docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md
Outdated
Show resolved
Hide resolved
- `loadbalancer.openstack.org/custom-tags` | ||
|
||
Allows customable loadbalancer tag configurable added during initial stage of loadbalancer creation. | ||
Tags are abitrary strings that can me added to loadbalancer and using this annotation allows adding of one or more custom tag to the `LoadBalancer`, `Listener` and `Pool` |
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.
I specifically asked to not do that, I do not think we should end up with 10 more annotations supported for each of the resource we create in CPO.
3d114cc
to
03c525d
Compare
Hi @KingDaemonX If you addressed something, you can resolve the related comments. |
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.
I realized we're missing tagging of floating IPs here. You should be able to add it here:
cloud-provider-openstack/pkg/openstack/loadbalancer.go
Lines 918 to 927 in 03c525d
func (lbaas *LbaasV2) createFloatingIP(msg string, floatIPOpts floatingips.CreateOpts) (*floatingips.FloatingIP, error) { | |
klog.V(4).Infof("%s floating ip with opts %+v", msg, floatIPOpts) | |
mc := metrics.NewMetricContext("floating_ip", "create") | |
floatIP, err := floatingips.Create(lbaas.network, floatIPOpts).Extract() | |
err = PreserveGopherError(err) | |
if mc.ObserveRequest(err) != nil { | |
return floatIP, fmt.Errorf("error creating LB floatingip: %s", err) | |
} | |
return floatIP, err | |
} |
Please note it's a Neutron resource again, so the tagging is done the same way as in case of the security groups (the string should be "floatingips", I check that in the API reference).
I'm trying to build this and run a bit of tests, might get back with more comments.
Okay, nevermind, after changing |
Ah, one more thing, we should add these Neutron tag operations to metrics. Here's how you do it when tagging: https://github.com/kubernetes/cloud-provider-openstack/blob/release-1.27/pkg/openstack/loadbalancer.go#L769-L774 Let's name the context |
yeah i get you |
03c525d
to
954ee65
Compare
i have made all the required changes to the PR @dulek |
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.
Seems like you've used security-group
instead of security-groups
. I'm fairly sure only the latter works, but please prove me wrong if you've tested it and it works.
Also seems like my remarks from this comment about metrics are not implemented.
i totally miss the remark on metric until now that you pointed it again |
Hm, I also don't like an idea that we need to tag all the resources at once. Neutron is modular and there is a possibility that it doesn't have tags module enabled, while octavia can have this module. If tags in neutron are not enabled, the OCCM will fail the reconciliation with an error. Also security groups and especially FIPs can have their own set of tags, e.g. FIP can be preallocated in advance and a special tag can be assigned on it. Once OCCM starts to manage this FIP, it will remove the tags assigned manually. This should not happen. |
954ee65
to
0236639
Compare
@KingDaemonX: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
0236639
to
10649fe
Compare
…ing service annotation
10649fe
to
fbd351b
Compare
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.
This version adds the metrics in a wrong place and using wrong IDs. I've pointed to that codeplace as an example where we did some tagging and metrics for another purpose, but the actual tagging should happen just after the resource is created.
mc = metrics.NewMetricContext("floating_ip_tag", "replace") | ||
_, err = neutrontags.ReplaceAll(network, "floatingips", port.ID, neutrontags.ReplaceAllOpts{Tags: tags}).Extract() | ||
if mc.ObserveRequest(err) != nil { | ||
return fmt.Errorf("failed to add tag %s to port %s of floating_ips: %v", tags, port.ID, 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.
This shouldn't be here, this function doesn't have anything to do with floating IPs.
Also this won't ever work, you specify port.ID
which will not match a floating IP.
mc = metrics.NewMetricContext("security_group_tag", "replace") | ||
_, err := neutrontags.ReplaceAll(network, "security_groups", port.ID, neutrontags.ReplaceAllOpts{Tags: tags}).Extract() | ||
if mc.ObserveRequest(err) != nil { | ||
return fmt.Errorf("failed to add tag %s to port %s: %v", tags, port.ID, 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.
I think this is wrong placement, this method applies the security group to a certain port. For now we should limit the SG tagging to the place we're creating it.
Also this won't ever work, you specify port.ID
which will not match a security group.
if _, err := neutrontags.ReplaceAll(lbaas.network, "floatingips", floatIP.ID, neutrontags.ReplaceAllOpts{Tags: tags}).Extract(); err != nil { | ||
return nil, fmt.Errorf("failed to add custom tags %s to floatingIPs %s with a projectID (%s)", tags, floatIP.ID, floatIP.ProjectID) | ||
} |
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.
This is missing the metrics addition. This is where you should add mc = metrics.NewMetricContext("floating_ip_tag", "replace")
and all the other stuff.
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.
I also realized we should only call this code when len(tags) > 0
.
if _, err := neutrontags.ReplaceAll(lbaas.network, "security-groups", lbSecGroupID, neutrontags.ReplaceAllOpts{Tags: tags}).Extract(); err != nil { | ||
return fmt.Errorf("failed to add custom tags %s to security group %s (%s)", tags, lbSecGroupID, lbSecGroupName) | ||
} |
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.
This is missing the mc = metrics.NewMetricContext("security_group_tag", "replace")
and then the subsequent ObserveRequest()
.
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.
Same here should only do tagging when len(tags) > 0
.
if _, err := neutrontags.ReplaceAll(lbaas.network, "floatingips", floatIP.ID, neutrontags.ReplaceAllOpts{Tags: tags}).Extract(); err != nil { | ||
return nil, fmt.Errorf("failed to add custom tags %s to floatingIPs %s with a projectID (%s)", tags, floatIP.ID, floatIP.ProjectID) | ||
} |
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.
I also realized we should only call this code when len(tags) > 0
.
if _, err := neutrontags.ReplaceAll(lbaas.network, "security-groups", lbSecGroupID, neutrontags.ReplaceAllOpts{Tags: tags}).Extract(); err != nil { | ||
return fmt.Errorf("failed to add custom tags %s to security group %s (%s)", tags, lbSecGroupID, lbSecGroupName) | ||
} |
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.
Same here should only do tagging when len(tags) > 0
.
True, we can do that and make sure that tagging will not happen when Neutron doesn't have the extension. Octavia is not modular, so it's just a matter of API version, but we should check for tags extension in Neutron case too. @KingDaemonX - we need to add a function checking this and only attempt to tag FIPs and SGs when it exists. This is an example of how to do it: cloud-provider-openstack/pkg/openstack/openstack.go Lines 475 to 484 in fdba36b
We need to check for
Current code (after my remarks will be addressed) is only tagging FIPs on creation. Same happens with SGs, we assume it's always us creating it, if |
PR needs rebase. 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. |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
Ah wait, this is indeed stale. I'll just close this, we have other PRs looking at this. |
/kind feat
What this PR does / why we need it:
this PR solves the add annotation for custom octavia listener tags problem
this is currently a draft PR that needs review, also i am a bit confuse on where to plug the method 😢
Which issue this PR fixes(if applicable):
fixes #2327