-
Notifications
You must be signed in to change notification settings - Fork 321
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
Net 6575- Modify Gateways Cleanup Job to cleanup v2 resources #3317
Conversation
80cedc2
to
0f65722
Compare
_ = c.k8sClient.Delete(context.Background(), gatewayClass) | ||
|
||
// make sure they're gone | ||
if err := backoff.Retry(func() error { |
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 do we do this twice? any reason to not just do the entire delete in the retry block to begin with?
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 since we're just ignoring the errors not having them in the retry block is an attempt to avoid spamming the same delete request while waiting for the finalizers to complete. But I just modified this code to make it into a loop.
|
||
// find the gateway class | ||
gatewayClass := &v2beta1.GatewayClass{} | ||
//TODO is the correct name for the gatewayclass? |
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 we are going to modify the configmap to have a gatewayclass in it, I will look for/make the story if you can add it as the TODO here
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.
//TODO: NET-6838
To pull the GatewayClassName from the Configmap
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.
Thank you that makes sense. Will Add.
0f65722
to
d03de67
Compare
@@ -8,6 +8,9 @@ import ( | |||
"errors" | |||
"flag" | |||
"fmt" | |||
|
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.
nit: extra newline
"github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" | ||
"github.com/hashicorp/consul-k8s/control-plane/subcommand" | ||
"github.com/hashicorp/consul-k8s/control-plane/subcommand/flags" | ||
) | ||
|
||
const ( | ||
gatewayConfigFilename = "/consul/config/config.yaml" | ||
resourceConfigFilename = "/consul/config/resources.json" |
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.
should these live in another package? we use them in the resources job so I'm wondering if they belong in the gateways
package as constants so we're not defining them twice across this and the resources job
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'd be in favor of pulling them into a shared package.
return 0 | ||
} | ||
|
||
func (c *Command) deleteV1GatewayClassAndGatewayClasConfig() error { |
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.
🙌
* cleanup existing gatewayclasses and gatewayclassconfigs * add tests for v2 * fixed test to actually run against v2 resources, found issue where loop was failing out early * add TODO with jira ticket * cleanup debug line * delete extra newline
Changes proposed in this PR:
How I've tested this PR:
kubectl logs job/consul-consul-gateway-cleanup -n consul
In order to make sure the cleanup job didn't actually error.
How I expect reviewers to test this PR:
Checklist: