Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ARO-11484] Fix fixetcd GA #4034

Open
wants to merge 105 commits into
base: master
Choose a base branch
from
Open

[ARO-11484] Fix fixetcd GA #4034

wants to merge 105 commits into from

Conversation

bitoku
Copy link
Contributor

@bitoku bitoku commented Jan 1, 2025 •
edited
Loading

Which issue this PR addresses:

Fixes https://issues.redhat.com/browse/ARO-11484

What this PR does / why we need it:

This PR fixes fixetcd GA.

  • Fix the label selector
  • Change the image to ubi9 because it got glibc error with ubi8.
  • Change the job to a pod. It only runs one pod, so there's no reason to use a job. Also its watcher returns a job object not a pod object.

Also I added e2e, but it takes so long and it can't run in parallel, so I make it regression test.
It doesn't run by default in CI.

E2E test is intended to ensure the master replacement SOP is valid because I couldn't reproduce the etcd issue with 100% possiblity.
I think it's enough solution until we find the reliable reproducible scenario.

Test plan for issue:

e2e

Is there any documentation that needs to be updated for this PR?

no

How do you know this will function as expected in production?

e2e, master replacement & fixetcd GA

kimorris27
kimorris27 requested changes Jan 2, 2025
View reviewed changes
Copy link
Contributor

@kimorris27 kimorris27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM, and it was thoughtful to add an E2E test. I made some small suggestions and have one other thing to point out.

There's a part of the user story that I don't see addressed in the PR: I want to add a conditional check for when the node IP address remains the same, and delete the existing etcd Pod if it's in a crashloop. Is that part of the story still needed?

@bitoku
Copy link
Contributor Author

bitoku commented Jan 2, 2025 •
edited
Loading

@kimorris27 Thank you for taking a look!

There's a part of the user story that I don't see addressed in the PR: I want to add a conditional check for when the node IP address remains the same, and delete the existing etcd Pod if it's in a crashloop. Is that part of the story still needed?

I don't think it's actually needed.
During the test, I got the error many times when the pod's IP address is unchanged, and fixetcd API fixed the issue.
Fixetcd just deletes the etcd member, but it seems when there's a change in etcd, etcd-operator automatically redeploys all etcd pods.

You might be able to reproduce it by running e2e. When you get 200 from the fixetcd api, the etcd pod should be CrashLoopBackoff.

kimorris27
kimorris27 requested changes Jan 3, 2025
View reviewed changes
Copy link
Contributor

@kimorris27 kimorris27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM given the responses to my original comments. The only issue I see now is that it looks like some unit tests in pkg/frontend need to be updated to reflect the changes to fixetcd.go, but maybe someone else will need to pick up this work first?

kimorris27
kimorris27 previously approved these changes Jan 3, 2025
View reviewed changes
@s-fairchild
Copy link
Collaborator

Have you tested this with a dev cluster?

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 7, 2025

Please rebase pull request.

@github-actions github-actions bot added the needs-rebase branch needs a rebase label Feb 7, 2025
stevekuznetsov and others added 27 commits February 19, 2025 14:28
@stevekuznetsov @s-fairchild
pkg/env: log MSI data-plane interactions (#4080)
56b135a 
* pkg/env: log MSI data-plane interactions

Signed-off-by: Steve Kuznetsov <[email protected]>

* clustermsi: send identityIDs, not delegatedResources

Signed-off-by: Steve Kuznetsov <[email protected]>

* go.mod: bump Azure/msi-dataplane

Signed-off-by: Steve Kuznetsov <[email protected]>

* *: update to use new Azure/msi-dataplane

Signed-off-by: Steve Kuznetsov <[email protected]>

* *: use policy.Policy construct for logging

Previously, we were substituting the entire http transport just to add
some client-side middleware. The Azure SDK for Go ships with default
transport parameters that we lost by doing this; furthermore, we were
not using the per-call policy options to model our middleware, as the
SDK expected.

Signed-off-by: Steve Kuznetsov <[email protected]>

* clustermsi: be smarter about logging responses

Signed-off-by: Steve Kuznetsov <[email protected]>

---------

Signed-off-by: Steve Kuznetsov <[email protected]>
@tsatam @s-fairchild
Add devops identity as Storage Blob Data Contributor to rpversion SA (#…
417249e 
…4094)
@stevekuznetsov @s-fairchild
go.mod: bump msi-dataplane to v0.4.0 (#4095)
9ea3419 
Signed-off-by: Steve Kuznetsov <[email protected]>
@stevekuznetsov @s-fairchild
bump golang-ci-lint (#4090)
75fa91c 
* Makefile: bump golangci-lint

Signed-off-by: Steve Kuznetsov <[email protected]>

* test/e2e: fixup bad Ginkgo assertions

ginkgo-linter: wrong error assertion. Consider using `Eventually(func(ctx context.Context) error {
	return project.VerifyProjectIsReady(ctx)
}).WithContext(ctx).WithTimeout(DefaultEventuallyTimeout).Should(Succeed())` instead (ginkgolinter)

Signed-off-by: Steve Kuznetsov <[email protected]>

* pkg/api: remove unnecessary nil checks

S1009: should omit nil check; len() for []string is defined as zero (gosimple)

Signed-off-by: Steve Kuznetsov <[email protected]>

* pkg/util: fix ineffectual compiler directive

SA9009: ineffectual compiler directive due to extraneous space:
"//  go:generate mockgen -destination=../mocks/$GOPACKAGE/client.go
github.com/Azure/msi-dataplane/pkg/dataplane Client" (staticcheck)

Signed-off-by: Steve Kuznetsov <[email protected]>

* *: fix non-constant format strings

printf: non-constant format string in call to (*github.com/sirupsen/logrus.Entry).Warnf (govet)

Signed-off-by: Steve Kuznetsov <[email protected]>

* cmd/migrate: add a program to refactor NewCloudError

Signed-off-by: Steve Kuznetsov <[email protected]>

* *: run the program to refactor NewCloudError

Signed-off-by: Steve Kuznetsov <[email protected]>

* pkg/api: refactor NewCloudError to take a static message

Signed-off-by: Steve Kuznetsov <[email protected]>

* Revert "cmd/migrate: add a program to refactor NewCloudError"

This reverts commit 8aa74d1f5d1f006d840e025bf025ce728cced0c4.

Signed-off-by: Steve Kuznetsov <[email protected]>

* pkg/frontend: fix error handling in fixetcd

Signed-off-by: Steve Kuznetsov <[email protected]>

---------

Signed-off-by: Steve Kuznetsov <[email protected]>
@aasserzo @s-fairchild
ARO-13921 Add cluster subnets' network security groups to List Cluste…
cd6983c 
…r Azure Resources output if cluster uses preconfiguredNSG (#4099)

* extended appendAzureNetworkResources() to fetch BYO NSGs

* fix existing tests, create BYO NSG tests

* fixed expected RG in mock

* fixed test

* reduced some nested conditions

* added validation for a subnet with no NSG
@Tof1973 @s-fairchild
[ARO-15116] DES missing Keyvault Access Policy on E2E retry (#4093)
38618bc 
* reuse kv for des if already exists
* advice what to do with invalid DES
* Inverse the if logic
* inverse also if logic to manage only errors
* NameAvailable nil is an error
* add debug log in cas of retry
* type, and replace debug by info
* remote code for testing...
@hawkowl @s-fairchild
make a single merge impl -- I'm not sure why I didn't do that in the …
5eb46ac 
…first place (#4096)
@rajdeepc2792 @s-fairchild
reuse env from the monitor object (#4101)
2741656
@gouthamMN @s-fairchild
go.mod: add OTEL audit library
7172de2
@gouthamMN @s-fairchild
Instrument Otel audit
afbe287
@gouthamMN @s-fairchild
address comments
c20e93b
@gouthamMN @s-fairchild
read queue size from RP-config
9ce80dd
@LiniSusan @s-fairchild
ARO-13380 - metrics: cwp status (#4002)
45d3a84
@pepedocs @s-fairchild
ARO-8425: Return ClientSecretCredential error (expired keys) as Cloud… (
4d026be 
#4098)

* ARO-8425: Return ClientSecretCredential error (expired keys) as CloudError

* Updated error message and added one more test case
@hlipsig @s-fairchild
Looks like version mismatch in golangci. This might fix
a24fb40
@hlipsig @s-fairchild
Corrected my dyslexic moment
c41dfef
@hlipsig @s-fairchild
replace skip-dirs with exclude dirs
284a1e7
@hlipsig @s-fairchild
Moved exclusions under issues
5750286
@hlipsig @s-fairchild
fix output to match docs
84c3795
@hlipsig @s-fairchild
I think it wanted single quotes on output
8638930
@hlipsig @s-fairchild
formats exepcts as list of formats, that's probably why this broke
9562a0b
@hlipsig @s-fairchild
Also fix the error that linting caught
b260ccb
@hlipsig @s-fairchild
Fixed trailing space in YAML for golang ci
944829e
@hlipsig @s-fairchild
Also removing ==, who let me code anyway
d7c12dc
@hlipsig @s-fairchild
adjusted the if statement correctly. Multitasking leads to failures, …
18d0820 
…testing locally is important
@hlipsig @s-fairchild
another trailing space
ec25cdd
@hlipsig @s-fairchild
another trailing space
9c75cd0 
part2
@s-fairchild s-fairchild mentioned this pull request Feb 19, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kimorris27 kimorris27 kimorris27 left review comments

@jewzaam jewzaam Awaiting requested review from jewzaam jewzaam is a code owner

@bennerv bennerv Awaiting requested review from bennerv bennerv is a code owner

@hawkowl hawkowl Awaiting requested review from hawkowl hawkowl is a code owner

@rogbas rogbas Awaiting requested review from rogbas rogbas is a code owner

@petrkotas petrkotas Awaiting requested review from petrkotas petrkotas is a code owner

@jharrington22 jharrington22 Awaiting requested review from jharrington22 jharrington22 is a code owner

@cblecker cblecker Awaiting requested review from cblecker cblecker is a code owner

@cadenmarchese cadenmarchese Awaiting requested review from cadenmarchese cadenmarchese is a code owner

@UlrichSchlueter UlrichSchlueter Awaiting requested review from UlrichSchlueter UlrichSchlueter is a code owner

@SudoBrendan SudoBrendan Awaiting requested review from SudoBrendan

@yjst2012 yjst2012 Awaiting requested review from yjst2012 yjst2012 is a code owner

@jaitaiwan jaitaiwan Awaiting requested review from jaitaiwan jaitaiwan is a code owner

@anshulvermapatel anshulvermapatel Awaiting requested review from anshulvermapatel anshulvermapatel is a code owner

@hlipsig hlipsig Awaiting requested review from hlipsig hlipsig is a code owner

@tiguelu tiguelu Awaiting requested review from tiguelu tiguelu is a code owner

@mociarain mociarain Awaiting requested review from mociarain mociarain is a code owner

@tsatam tsatam Awaiting requested review from tsatam tsatam is a code owner

@fahlmant fahlmant Awaiting requested review from fahlmant fahlmant is a code owner

@sankur-codes sankur-codes Awaiting requested review from sankur-codes sankur-codes is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
needs-rebase branch needs a rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.