Pilot executes `cassandra` directly, bypassing the container image entry point #347

wallrj · 2018-05-08T15:21:48Z

Cassandra Pilot now executes cassandra directly, bypassing the Docker image entrypoint
Added a configuration API with which to override configuration values in yaml and properties config files.
This replaces the experimental search / replace code for seed_provider and snitch.
This also the use of environment variables to configure the Cassandra node. Instead, those configuration options are now supplied as Pilot command line arguments and are written to the config files in a pre-start pilot hook.
Added an integration test so that i can see this mechanism working, without having to wait for e2e tests.

Release note:

NONE

jetstack-bot · 2018-05-08T15:21:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: munnerz

Assign the PR to them by writing /assign @munnerz in a comment when ready.

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

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

* to allow integration testing of pilot process without having to set up a leader election config map.

* An integration test to launch the Cassandra Pilot and ensure that it writes configuration files before launching the cassandra sub-process. * Exec the pilot with leader-election disabled. * A script to download suitable integration test binaries. * A helper to create a .kube/config for the pilot to use.

* Remove the environment variables that were previously used to modify the configuration.

…e2e tests accordingly.

jetstack-bot · 2018-06-08T00:52:20Z

@wallrj: 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.

munnerz · 2018-06-12T09:50:37Z

pkg/pilot/genericpilot/options.go

@@ -179,5 +180,7 @@ func (o *Options) Pilot() (*GenericPilot, error) {
 func (o *Options) AddFlags(flags *pflag.FlagSet) {
 	flags.StringVar(&o.PilotName, "pilot-name", "", "The name of this Pilot. If not specified, an auto-detected name will be used.")
 	flags.StringVar(&o.PilotNamespace, "pilot-namespace", "", "The namespace the corresponding Pilot resource for this Pilot exists within.")
+	flags.BoolVar(&o.LeaderElect, "leader-elect", true, ""+


I'm pretty nervous exposing this option to users - it should be defined by the Pilot (e.g. elasticsearch-pilot etc) whether leader election needs to be performed.
Disabling this could cause fundamental breakages to how the Pilot operates, so I don't think it should be a user exposed option.

Is this because you don't use the leader election primitives in the cassandra pilot? If so, can you change this to only be configurable by the Pilot code (i.e. not a user facing option)?

munnerz · 2018-06-12T09:51:26Z

internal/test/util/testfs/testfs.go

+	err := os.MkdirAll(outPath, os.ModePerm)
+	require.NoError(tfs.t, err)
+	return outPath
+}


What's the reasoning for this package? It seems to wrap ioutil, but with some extras in there?
It'd be great if we can just use ioutil for this, to save having additional code to maintain.

https://golang.org/pkg/io/ioutil/#TempDir

munnerz · 2018-06-12T09:53:01Z

internal/test/util/testfs/testfs.go

+}
+
+func New(t *testing.T) *TestFs {
+	d := fmt.Sprintf(".test/%s", t.Name())


Is there a reason for specifying a custom base, instead of relying on ioutil to pick the correct OS-specific tmp dir?

munnerz · 2018-06-12T09:55:22Z

hack/download-integration-test-binaries.sh

+ETCD_VERSION=v3.2.10
+ETCD_URL="https://storage.googleapis.com/etcd"
+
+KUBE_VERSION_URL="https://storage.googleapis.com/kubernetes-release/release/stable-1.10.txt"


Can we pin this to a specific patch release of 1.10? Or otherwise, clearly print out the exact version returned here in the test logs, so we can debug failures caused by patch version changes.

munnerz · 2018-06-12T10:00:24Z

test/integration/pilot_test.go

+	)
+	t.Log("stdout", stdout)
+	t.Log("stderr", stderr)
+	require.NoError(t, err)


From here up to the start of the function should be in some shared function - this is code that is not specific to this test, or Cassandra.

Also, we should add some comments to this explaining why we are registering a CRD, and what is and isn't okay to test whilst using a CRD (because a lot of code for e.g. validation won't be run, we should clearly define bounds for what is and isn't okay to test). As we spoke about before, I think we should look to remove this code ASAP in favour of launching navigator-apiserver itself.

Also, given what we're trying to test here, would it be possible to implement this whole test as a unit test? I'm not sure why we need to bring up the entire process to verify that a config file is written ❓

munnerz · 2018-06-12T10:12:53Z

docs/cassandra.rst

+This ``pilot`` process connects to the API server to:
+get extra configuration settings,
+report the status of this C* node, and to
+perform leader election of a single pilot in the cluster.


(nit) Bulleted list 😄

munnerz · 2018-06-12T10:13:23Z

docs/cassandra.rst

+
+The ``pilot`` overrides the following keys in the default ``/etc/cassandra/cassandra.yaml`` file:
+
+* ``cluster_name``: This will be set to match the name of the corresponding ``CassandraCluster`` resource in the API server.


Just a thought: is this safe to do, given two clusters may have the same name (in two different namespaces)

munnerz · 2018-06-12T10:14:17Z

docs/cassandra.rst

+  The ``seed_provider.*.seeds`` sub key will be emptied.
+  This is to avoid the risk of nodes mistakenly joining the cluster as seeds if the seed provider service is temporarily unavailable.
+
+The ``pilot`` also overwrites ``cassandra-rackdc.properties`` with values derived from the ``CassandraCluster.Spec.Nodepools`` (see :ref:`availability-zones-cassandra`).


Nodepools -> NodePools

munnerz · 2018-06-12T10:15:54Z

Gopkg.lock

+  name = "github.com/stretchr/testify"
+  packages = ["assert","require"]
+  revision = "12b6f73e6084dad08a7c6e575284b177ecafbc71"
+  version = "v1.2.1"


How come we need testify? We've not used it in any of our other tests so far

munnerz · 2018-06-12T10:16:18Z

Gopkg.lock

+  name = "github.com/fsnotify/fsnotify"
+  packages = ["."]
+  revision = "c2828203cd70a50dcccfb2761f8b1f8ceef9a8e9"
+  version = "v1.4.7"


Any idea what introduces a need for fsnotify? 🤔 viper?

jetstack-bot added do-not-merge/work-in-progress release-note-none labels May 8, 2018

jetstack-bot requested a review from munnerz May 8, 2018 15:21

jetstack-bot added the size/M label May 8, 2018

wallrj force-pushed the 346-exec-cassandra branch from 27db6bc to 7e8965b Compare May 17, 2018 08:25

jetstack-bot added size/L and removed size/M labels May 17, 2018

wallrj force-pushed the 346-exec-cassandra branch from 7e8965b to 34c30a3 Compare May 21, 2018 16:58

jetstack-bot added size/XXL and removed size/L labels May 21, 2018

wallrj force-pushed the 346-exec-cassandra branch from 34c30a3 to 757e72f Compare May 22, 2018 08:38

wallrj force-pushed the 346-exec-cassandra branch 4 times, most recently from 144c3d2 to 381c7ea Compare June 6, 2018 09:31

wallrj mentioned this pull request Jun 6, 2018

Use local volumes instead of hostpath volumes #366

Merged

wallrj force-pushed the 346-exec-cassandra branch from 381c7ea to b05c230 Compare June 6, 2018 11:29

wallrj changed the title ~~WIP: Pilot executes cassandra directly, bypassing the container image entry point~~ Pilot executes cassandra directly, bypassing the container image entry point Jun 6, 2018

jetstack-bot removed the do-not-merge/work-in-progress label Jun 6, 2018

wallrj added 2 commits June 7, 2018 11:17

Add a pilot --leader-elect flag

69a59e5

* to allow integration testing of pilot process without having to set up a leader election config map.

A helper for generating test files and directories.

20deca2

wallrj force-pushed the 346-exec-cassandra branch 2 times, most recently from 8cfae12 to add5b6c Compare June 7, 2018 10:33

wallrj added 6 commits June 7, 2018 11:45

Add Pilot command line arguments for all the configuration values

330437b

* Remove the environment variables that were previously used to modify the configuration.

A configuration package for modifying yaml and properties files.

2426e2c

Use the Configuration API to modify configuration files.

a25029d

Run the Cassandra process as the Cassandra user (999) and update the …

74bb244

…e2e tests accordingly.

documentation

641310c

dep ensure -v

015c813

wallrj force-pushed the 346-exec-cassandra branch from add5b6c to 015c813 Compare June 7, 2018 10:45

jetstack-bot added the needs-rebase label Jun 8, 2018

munnerz reviewed Jun 12, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pilot executes `cassandra` directly, bypassing the container image entry point #347

Pilot executes `cassandra` directly, bypassing the container image entry point #347

wallrj commented May 8, 2018 •

edited

Loading

jetstack-bot commented May 8, 2018

jetstack-bot commented Jun 8, 2018

munnerz Jun 12, 2018

munnerz Jun 12, 2018

munnerz Jun 12, 2018

munnerz Jun 12, 2018

munnerz Jun 12, 2018

munnerz Jun 12, 2018

munnerz Jun 12, 2018

munnerz Jun 12, 2018

munnerz Jun 12, 2018

munnerz Jun 12, 2018


		The ``pilot`` overrides the following keys in the default ``/etc/cassandra/cassandra.yaml`` file:

		* ``cluster_name``: This will be set to match the name of the corresponding ``CassandraCluster`` resource in the API server.

Pilot executes cassandra directly, bypassing the container image entry point #347

Are you sure you want to change the base?

Pilot executes cassandra directly, bypassing the container image entry point #347

Conversation

wallrj commented May 8, 2018 • edited Loading

jetstack-bot commented May 8, 2018

jetstack-bot commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pilot executes `cassandra` directly, bypassing the container image entry point #347

Pilot executes `cassandra` directly, bypassing the container image entry point #347

wallrj commented May 8, 2018 •

edited

Loading