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.

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

Replacement logic should ignore process groups that are in maintenance mode #1711

Merged
merged 3 commits into from
Jul 7, 2023

Conversation

sbodagala
Copy link
Contributor

@sbodagala sbodagala commented Jun 27, 2023

Description

Modify operator replacement logic to ignore process groups that are in maintenance mode.

Type of change

Please select one of the options below.

  • New feature (non-breaking change which adds functionality)

Discussion

Testing

Manual testing.

Documentation

Follow-up

@foundationdb-ci
Copy link

Result of fdb-kubernetes-operator-pr on Linux CentOS 7

  • Commit ID: efaa4ae
  • Duration 4:06:52
  • Result: ❌ FAILED
  • Error: Error while executing command: if $fail_test; then exit 1; fi. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

Copy link
Member

@johscheuer johscheuer left a comment

Choose a reason for hiding this comment

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

The changes look good. Could we add a test for this in the operator test suite to make sure we have an e2e test for this behaviour?

controllers/replace_failed_process_groups_test.go Outdated Show resolved Hide resolved
@sbodagala
Copy link
Contributor Author

The changes look good. Could we add a test for this in the operator test suite to make sure we have an e2e test for this behaviour?

I am not sure how to write a stand alone test that tests the functionality of replacing failed process groups? This test "a process group has no address assigned and should be removed" in "operator_test.go" appears to test this functionality but I am not sure. Any other tests here that test this functionality? If not, I think I am going to need pointers as to how to write this test. Thanks!

@foundationdb-ci
Copy link

Result of fdb-kubernetes-operator-pr on Linux CentOS 7

  • Commit ID: a4d4bb0
  • Duration 4:07:10
  • Result: ❌ FAILED
  • Error: Error while executing command: if $fail_test; then exit 1; fi. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

Copy link
Member

@johscheuer johscheuer left a comment

Choose a reason for hiding this comment

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

I am not sure how to write a stand alone test that tests the functionality of replacing failed process groups? This test "a process group has no address assigned and should be removed" in "operator_test.go" appears to test this functionality but I am not sure. Any other tests here that test this functionality? If not, I think I am going to need pointers as to how to write this test. Thanks!

We have some examples here: https://github.com/FoundationDB/fdb-kubernetes-operator/blob/main/controllers/replace_failed_process_groups_test.go#L634-L950. Let me know if you need some additional guidance.

Copy link
Member

@johscheuer johscheuer left a comment

Choose a reason for hiding this comment

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

Changes LGTM 👍

@sbodagala
Copy link
Contributor Author

I am not sure how to write a stand alone test that tests the functionality of replacing failed process groups? This test "a process group has no address assigned and should be removed" in "operator_test.go" appears to test this functionality but I am not sure. Any other tests here that test this functionality? If not, I think I am going to need pointers as to how to write this test. Thanks!

We have some examples here: https://github.com/FoundationDB/fdb-kubernetes-operator/blob/main/controllers/replace_failed_process_groups_test.go#L634-L950. Let me know if you need some additional guidance.

But that's where I have added the test "with maintenance mode enabled"? I am confused now.

@johscheuer
Copy link
Member

I'm going to merge the changes once the CI completes.

@foundationdb-ci
Copy link

Result of fdb-kubernetes-operator-pr on Linux CentOS 7

  • Commit ID: c85c049
  • Duration 2:19:12
  • Result: ❌ FAILED
  • Error: Error while executing command: if $fail_test; then exit 1; fi. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@johscheuer
Copy link
Member

Result of fdb-kubernetes-operator-pr on Linux CentOS 7

  • Commit ID: c85c049
  • Duration 2:19:12
  • Result: ❌ FAILED
  • Error: Error while executing command: if $fail_test; then exit 1; fi. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

The test failure is unrelated to those changes and is the result of a flaky test (we have an issue to track and fix the test).

@johscheuer johscheuer merged commit 4f14519 into FoundationDB:main Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants