Fix accidental ignition and stuck graceful termination #2212

tnozicka · 2024-11-28T14:18:00Z

Description of your changes:
This PR fixes the ignition touching the file before it's actually ignited and adds e2e coverage to prevent it from happening again. (Looks like it got broken in the early iterations.)

It also fixes stuck graceful termination before ignition is done because fixing one surfaces the other is broken and they can't be split without disabling some tests. (For this reason the ignition file is changed twice - best viewed as an overall diff then "by commit"). It also fixes graceful termination after ignition that was broken because of the manager agent container wasn't using exec. (@rzetelskik found that previously and it also failed in the test I wrote.)

Which issue is resolved by this Pull Request:
Resolves #2213

scylla-operator-bot · 2024-11-28T14:18:03Z

@tnozicka: GitHub didn't allow me to request PR reviews from the following users: tnozicka.

Note that only scylladb members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Description of your changes:
This PR fixes the ignition touching the file before it's actually ignited and add e2e coverage to prevent it from happening again. (Looks like it got broken in the early iterations.)

Which issue is resolved by this Pull Request:
Resolves #

/cc

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-sigs/prow repository.

tnozicka · 2024-11-28T15:08:21Z

(the e2e test on it's own failed as expected - the run was https://prow.scylla-operator.scylladb.com/view/gs/scylla-operator-prow/pr-logs/pull/scylladb_scylla-operator/2212/pull-scylla-operator-master-e2e-gke-serial/1862139318282227712#1:test-build-log.txt%3A994)

 NodeConfig Optimizations [It] should correctly project state for each scylla pod [Serial]
github.com/scylladb/scylla-operator/test/e2e/set/nodeconfig/nodeconfig_optimizations.go:194
  [FAILED] container in Pod "basic-dr65w-us-east-1-us-east-1a-0" don't match the expected state
  Expected
      <map[string]bool | len:5>: {
          "sidecar-injection": true,
          "scylla": true,
          "scylla-manager-agent": true,
          "scylladb-api-status-probe": true,
          "scylladb-ignition": false,
      }
  to have {key: value}
      <map[interface {}]interface {} | len:1>: {
          <string>"scylla": <bool>false,
      }
  In [It] at: github.com/scylladb/scylla-operator/test/e2e/set/nodeconfig/nodeconfig_optimizations.go:342 @ 11/28/24 14:57:40.478
  Full Stack Trace
    github.com/scylladb/scylla-operator/test/e2e/set/nodeconfig.init.func2.4()
    	github.com/scylladb/scylla-operator/test/e2e/set/nodeconfig/nodeconfig_optimizations.go:342 +0x2353

rzetelskik

one typo
lgtm
/assign zimnx

rzetelskik · 2024-11-28T15:53:15Z

test/e2e/set/nodeconfig/nodeconfig_optimizations.go

+					o.HaveKeyWithValue(naming.ScyllaContainerName, false),
+					o.HaveKeyWithValue(naming.ScyllaDBIgnitionContainerName, false),
+				),
+				fmt.Sprintf("container in Pod %q don't match the expected state", pod.Name),


nit

Suggested change

fmt.Sprintf("container in Pod %q don't match the expected state", pod.Name),

fmt.Sprintf("containers in Pod %q don't match the expected state", pod.Name),

ok, I suppose container(s) as it's 1-N

either works

tnozicka · 2024-11-29T08:24:47Z

now that we actually wait for ignition - the tuning took over 9 minutes which made the job timeout
https://gcsweb.scylla-operator.scylladb.com/gcs/scylla-operator-prow/pr-logs/pull/scylladb_scylla-operator/2212/pull-scylla-operator-master-e2e-gke-serial/1862151638882455552/artifacts/e2e/cluster-0/namespaces/e2e-test-nodeconfig-z4wpb-0-j4m2b/pods/basic-8l7bn-us-east-1-us-east-1a-0/scylladb-ignition.current

tnozicka · 2024-12-02T08:27:49Z

blocked on #2216

tnozicka · 2024-12-09T13:36:01Z

#2246 landed
/test all

zimnx

One comment, otherwise lgtm. Please fix PR description, one sentence contains duplicated parts.

test/e2e/set/scyllacluster/scyllacluster_restarts.go

tnozicka · 2024-12-11T08:35:03Z

Please fix PR description, one sentence contains duplicated parts.

I fixed the typos and added more context, but one fix is for graceful termination before ignition and the other fix is after ignition.

zimnx

lgtm
/assign rzetelskik

scylla-operator-bot · 2024-12-11T08:57:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rzetelskik, tnozicka, zimnx

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:

~~OWNERS~~ [tnozicka,zimnx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rzetelskik

one comment/question, rest lgtm

test/e2e/set/nodeconfig/nodeconfig_optimizations.go

rzetelskik · 2024-12-11T12:28:03Z

/lgtm

tnozicka · 2024-12-11T16:38:14Z

bug #2249
/retest

tnozicka added kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels Nov 28, 2024

tnozicka force-pushed the fix-ignition branch from d9caa3b to 995e3a5 Compare November 28, 2024 14:19

tnozicka force-pushed the fix-ignition branch from 995e3a5 to b51b4b3 Compare November 28, 2024 15:08

tnozicka changed the title ~~[WIP] Fix accidental ignition~~ Fix accidental ignition Nov 28, 2024

scylla-operator-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 28, 2024

tnozicka requested review from rzetelskik and zimnx and removed request for rzetelskik November 28, 2024 15:19

rzetelskik approved these changes Nov 28, 2024

View reviewed changes

scylla-operator-bot bot assigned zimnx Nov 28, 2024

zimnx approved these changes Nov 28, 2024

View reviewed changes

tnozicka changed the title ~~Fix accidental ignition~~ [WIP] Fix accidental ignition Nov 29, 2024

scylla-operator-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 29, 2024

tnozicka mentioned this pull request Dec 2, 2024

Tuning takes 9 minutes for a single node on GKE #2216

Closed

tnozicka force-pushed the fix-ignition branch from d98bd00 to 15d00e2 Compare December 2, 2024 12:11

scylla-operator-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 2, 2024

tnozicka force-pushed the fix-ignition branch from 911d5b0 to 3533f6f Compare December 3, 2024 15:58

scylla-operator-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 3, 2024

tnozicka force-pushed the fix-ignition branch from a491ba3 to 4b18dbf Compare December 4, 2024 08:33

tnozicka force-pushed the fix-ignition branch 2 times, most recently from 2c8f717 to baa2b8a Compare December 9, 2024 12:58

tnozicka added 2 commits December 9, 2024 15:13

Fix accidental ignition

bc33148

Add context to ExecWithOptions

4e31f5f

tnozicka force-pushed the fix-ignition branch 3 times, most recently from 2d63a42 to 42ca696 Compare December 10, 2024 08:07

scylla-operator-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 10, 2024

tnozicka force-pushed the fix-ignition branch from 42ca696 to c98af31 Compare December 10, 2024 10:17

tnozicka changed the title ~~[WIP] Fix accidental ignition and stuck graceful termination~~ Fix accidental ignition and stuck graceful termination Dec 10, 2024

scylla-operator-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 10, 2024

tnozicka requested review from rzetelskik and zimnx December 10, 2024 12:31

zimnx reviewed Dec 10, 2024

View reviewed changes

test/e2e/set/scyllacluster/scyllacluster_restarts.go Outdated Show resolved Hide resolved

Fix ScyllaDB graceful termination

978875b

tnozicka force-pushed the fix-ignition branch from c98af31 to 978875b Compare December 11, 2024 08:33

zimnx approved these changes Dec 11, 2024

View reviewed changes

scylla-operator-bot bot assigned rzetelskik Dec 11, 2024

rzetelskik reviewed Dec 11, 2024

View reviewed changes

test/e2e/set/nodeconfig/nodeconfig_optimizations.go Show resolved Hide resolved

scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2024

scylla-operator-bot bot merged commit 91b1fe0 into scylladb:master Dec 11, 2024
12 checks passed

tnozicka deleted the fix-ignition branch December 11, 2024 18:07

This was referenced Dec 11, 2024

[v1.15] Fix graceful termination, early ignition and related tests #2250

Merged

Graceful termination is not working for ScyllaCluster containers #2239

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix accidental ignition and stuck graceful termination #2212

Fix accidental ignition and stuck graceful termination #2212

tnozicka commented Nov 28, 2024 •

edited

Loading

scylla-operator-bot bot commented Nov 28, 2024

tnozicka commented Nov 28, 2024 •

edited

Loading

rzetelskik left a comment

rzetelskik Nov 28, 2024 •

edited

Loading

tnozicka Nov 29, 2024

rzetelskik Nov 29, 2024

tnozicka Dec 3, 2024

tnozicka commented Nov 29, 2024

tnozicka commented Dec 2, 2024

tnozicka commented Dec 9, 2024

zimnx left a comment

tnozicka commented Dec 11, 2024

zimnx left a comment

scylla-operator-bot bot commented Dec 11, 2024

rzetelskik left a comment

rzetelskik commented Dec 11, 2024

tnozicka commented Dec 11, 2024

	fmt.Sprintf("container in Pod %q don't match the expected state", pod.Name),
	fmt.Sprintf("containers in Pod %q don't match the expected state", pod.Name),

Fix accidental ignition and stuck graceful termination #2212

Fix accidental ignition and stuck graceful termination #2212

Conversation

tnozicka commented Nov 28, 2024 • edited Loading

scylla-operator-bot bot commented Nov 28, 2024

tnozicka commented Nov 28, 2024 • edited Loading

rzetelskik left a comment

Choose a reason for hiding this comment

rzetelskik Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

tnozicka Nov 29, 2024

Choose a reason for hiding this comment

rzetelskik Nov 29, 2024

Choose a reason for hiding this comment

tnozicka Dec 3, 2024

Choose a reason for hiding this comment

tnozicka commented Nov 29, 2024

tnozicka commented Dec 2, 2024

tnozicka commented Dec 9, 2024

zimnx left a comment

Choose a reason for hiding this comment

tnozicka commented Dec 11, 2024

zimnx left a comment

Choose a reason for hiding this comment

scylla-operator-bot bot commented Dec 11, 2024

rzetelskik left a comment

Choose a reason for hiding this comment

rzetelskik commented Dec 11, 2024

tnozicka commented Dec 11, 2024

tnozicka commented Nov 28, 2024 •

edited

Loading

tnozicka commented Nov 28, 2024 •

edited

Loading

rzetelskik Nov 28, 2024 •

edited

Loading