Apply all resources in a single kubectl call #872

samb1729 · 2017-12-17T13:28:41Z

In an attempt to make full-syncs as fast as possible, this change combines all yamels into a big multidoc and attempts to apply that with a single kubectl call.

In the event that the mega-apply fails, fallback to applying yamels one at a time.

Obligatory random cleanups included at no extra cost.

rade · 2017-12-17T13:50:37Z

Does this make flux less tolerant of

syntax errors in individual yamls
failures in applying a particular yaml

?

samb1729 · 2017-12-17T13:55:06Z

Yes (to both), fallback yet to be implemented.

I'm trying to figure out if there's some way to get the best of both worlds (single kubectl apply that doesn't stop at the first failure), but if that doesn't work we can always just attempt the mega-apply and go back to applying yamels one at a time if that fails.

squaremo · 2017-12-18T13:11:06Z

Shout-out for the great commit messages ⭐

squaremo · 2017-12-18T13:26:07Z

Does this make syncs appreciably faster? It's one exec vs ~100, of course, but I would expect most of the latency to be from the network round-trips kubectl does on each apply.

I'm also worried that there's much more chance of running into the limitation that caused #826 (which will be built into kubectl). It'd be quite easy to accumulate 64k of manifests. Granted, it will fall back to doing things one by one.

rade · 2017-12-18T13:31:47Z

Does this make syncs appreciably faster?

Some measurements would be nice.

squaremo · 2017-12-18T13:45:40Z

Probably the best place to get measurements is by running in dev :) We can do that by ignoreing the fluxd deployment, applying a config to use an image built from this branch, and looking at the metrics.

Since the namespace must be specified in the parsed manifest for us to retrieve it, we don't need to set this flag. Kubectl figures it out on its own.

These values are never actually used. They look like they are, but their only usage is immediately reassigned.

This change to the interface facilitates creating an Applier that combines all yamels into a multidoc to be applied in one call to kubectl. Note that the deleted test was testing behaviour that exists nowhere in flux. All references to `SyncAction` in the code contain only a single change, so the test was unnecessary.

Since the namespace flag is no longer needed for kubectl, and since the tests don't actually need the object name, we can pass a byte slice and simplify a bit. This change guides us towards doing the single mega-apply without smushing bytes into an apiObject in a kludgey way.

squaremo

Please rebut or adapt at your discretion.

cluster/kubernetes/kubernetes.go

-		return "default"
-	}
-	return obj.Metadata.Namespace
+func (o *apiObject) hasDefaultNamespace() bool {


cluster/kubernetes/kubernetes.go

 	version    string // string response for the version command.
 	logger     log.Logger
 	sshKeyRing ssh.KeyRing
+
+	sync.Mutex


cluster/kubernetes/release.go

-	cmd.Stderr = c.stderr
-	return cmd
+func (c *Kubectl) execute(logger log.Logger, errs cluster.SyncError) {
+	defer c.changeSet.clear()


cluster/kubernetes/release.go

+	}
+}
+
+type executeSet struct {


cmd/fluxd/main.go

@@ -146,7 +146,7 @@ func main() {
 	var clusterVersion string
 	var sshKeyRing ssh.KeyRing
 	var k8s cluster.Cluster
-	var image_creds func() registry.ImageCreds
+	var imageCreds func() registry.ImageCreds


cluster/kubernetes/release.go

+		return
+	}
+	// Attempt to apply everything in s at once, else fallback to applying one at a time.
+	if err := c.doCommand(logger, s.rw, s.cmd...); err != nil {


cluster/kubernetes/release.go

+			set.stage(obj)
+		}
+
+		c.exec(logger, defaultSet, cmd, errs)


As much as I love finding places to put channels, in this case it is entirely unnecessary. The only use of actionc can be replaced with a mutex, and any future methods can just compete for the lock instead of competing for a channel write.

This brings back the previous ordering without doing a sort. Since we no longer apply objects one at a time, we don't need an id.

This command was simply testing the mock.

We have this enormous block of nothingness all for satisying the requirements of a constructor we don't even need to use.

In some not-so-well-defined cases, applying an object to the default namespace can fail when its yaml doesn't explicitly specify the namespace. The simplest way to overcome this is to separate them out and pass the namespace flag to kubectl.

The ordering of application/deletion of namespaces before/after namespaced resources is specific to kubernetes and so does not belong in the generic sync package. This ordering is now implicitly handled by the order in which commands are run in the kubernetes package.

It occurred to me that there's no good reason this state should be stored within the struct that happens to implement the Applier interface. The state can be built in the Sync method and then garbage collected at the end.

squaremo

LGTM, thanks for persevering Sam.

cluster/kubernetes/kubernetes.go

 	}
-	return <-errc
+	c.applier.apply(logger, cs, errs)


samb1729 force-pushed the kubectl-megapply branch from 3bc99d5 to 4cae92f Compare December 17, 2017 13:32

samb1729 force-pushed the kubectl-megapply branch from 569fc1d to 6bbad23 Compare December 17, 2017 17:45

samb1729 requested a review from squaremo December 17, 2017 17:47

samb1729 changed the title ~~[WIP] Apply all resources in a single kubectl call~~ Apply all resources in a single kubectl call Dec 17, 2017

Sam Broughton added 5 commits December 21, 2017 14:09

Remove namespace flag from kubectl apply/delete calls

a5f18ff

Since the namespace must be specified in the parsed manifest for us to retrieve it, we don't need to set this flag. Kubectl figures it out on its own.

Remove unused apiObject fields

5bd8f40

Remove Stdout/err fields from Kubectl struct

40b23d5

These values are never actually used. They look like they are, but their only usage is immediately reassigned.

samb1729 force-pushed the kubectl-megapply branch from 46d37b2 to f2b6463 Compare December 21, 2017 12:34

squaremo suggested changes Dec 21, 2017

View reviewed changes

squaremo reviewed Dec 21, 2017

View reviewed changes

cluster/kubernetes/release.go Outdated

return

}

// Attempt to apply everything in s at once, else fallback to applying one at a time.

if err := c.doCommand(logger, s.rw, s.cmd...); err != nil {

This comment was marked as abuse.

Sign in to view

squaremo reviewed Dec 21, 2017

View reviewed changes

cluster/kubernetes/release.go Outdated

set.stage(obj)

}

c.exec(logger, defaultSet, cmd, errs)

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

Sam Broughton added 9 commits December 22, 2017 16:38

Replace actionc with a mutex

92e60a8

As much as I love finding places to put channels, in this case it is entirely unnecessary. The only use of actionc can be replaced with a mutex, and any future methods can just compete for the lock instead of competing for a channel write.

Store staged objects in a slice rather than a map

0a6ef34

This brings back the previous ordering without doing a sort. Since we no longer apply objects one at a time, we don't need an id.

Replace delete and apply with doCommand

8419cc3

Deduplicate delete/apply logic

687b704

Remove command order test

3000dcd

This command was simply testing the mock.

Fallback to applying yamels one-by-one if mass apply fails

7dbbd58

Remove mockClientSet noise

1d010ee

We have this enormous block of nothingness all for satisying the requirements of a constructor we don't even need to use.

Remove error return value from NewCluster

23c53d4

Camelify image_creds in main.go

ab35632

samb1729 force-pushed the kubectl-megapply branch from f2b6463 to 9884a57 Compare December 22, 2017 16:05

Factor out common bits of execute

68db839

samb1729 force-pushed the kubectl-megapply branch from 9884a57 to 68db839 Compare December 22, 2017 20:22

Sam Broughton added 2 commits December 28, 2017 18:25

Remove state from kubectl struct

c113bd1

It occurred to me that there's no good reason this state should be stored within the struct that happens to implement the Applier interface. The state can be built in the Sync method and then garbage collected at the end.

squaremo approved these changes Jan 2, 2018

View reviewed changes

cluster/kubernetes/kubernetes.go Outdated

}

return <-errc

c.applier.apply(logger, cs, errs)

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

Take cluster lock as late as possible

8c3f720

samb1729 merged commit 0972ee9 into master Jan 2, 2018

samb1729 deleted the kubectl-megapply branch January 2, 2018 13:42

squaremo mentioned this pull request Aug 19, 2021

reset sync errors on successful sync #3156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply all resources in a single kubectl call #872

Apply all resources in a single kubectl call #872

samb1729 commented Dec 17, 2017 •

edited

Loading

rade commented Dec 17, 2017 •

edited

Loading

samb1729 commented Dec 17, 2017 •

edited

Loading

squaremo commented Dec 18, 2017

squaremo commented Dec 18, 2017

rade commented Dec 18, 2017

squaremo commented Dec 18, 2017

squaremo left a comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

squaremo left a comment

This comment was marked as abuse.

This comment was marked as abuse.

Apply all resources in a single kubectl call #872

Apply all resources in a single kubectl call #872

Conversation

samb1729 commented Dec 17, 2017 • edited Loading

rade commented Dec 17, 2017 • edited Loading

samb1729 commented Dec 17, 2017 • edited Loading

squaremo commented Dec 18, 2017

squaremo commented Dec 18, 2017

rade commented Dec 18, 2017

squaremo commented Dec 18, 2017

squaremo left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

squaremo left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

samb1729 commented Dec 17, 2017 •

edited

Loading

rade commented Dec 17, 2017 •

edited

Loading

samb1729 commented Dec 17, 2017 •

edited

Loading