add multi-part yaml support to GetChanges for the bundle facade

[cderici]

Description

pylibjuju uses the bundle facade to get the changes for a bundle to be deployed. However, currently the GetChanges function that the facade uses only handles single-part yaml files, so this PR adds the support for multi-part yaml handling (which the Juju cli uses) to enable the use of overlays in bundles.

QA steps

  $ cd juju/apiserver/facades/client/bundle/
  $ go test -gocheck.f TestGetChangesWithOverlaysV6
  $ go test -gocheck.f TestGetChangesWithOverlaysV1_5

Notes & Discussion

This PR is needed for juju/python-libjuju#566, which adds the support for overlays in deploying bundles.

[achilleasa]

The code as-is should work fine for the majority of bundles. However, there are some (non-trivial) edge cases that you need to address before landing this:

1. Juju deploy allows you to specify a base bundle (+optional overlay(s) if you use the multi-doc syntax) plus any number of explicitly defined overlay files (e.g. juju deploy ./multidoc.yaml --overlay foo.yaml --overlay bar.yaml). You can technically concatenate them client-side before calling the facade (just add them as extra docs after the base). However, (there is a tricky bit here:
2. Bundles support an "include" directive (search for include-file here). The include path is relative to the overlay location which is why you see this mechanism with multiple bundle data sources. Since the facade logic cannot report back errors like "give me file X" you will need to emulate this logic and expand include-{file,base64} directives client-side where you can access the files.
3. Bundles/overlays can reference local charms (again, paths are relative to the thing that references them). I am not 100% sure how this can be made to work easily with the python client.

For point 2, you can take a look at the QA steps here to get a better idea about how it is supposed to work.

Note however that when the server-side bundle deployment get implemented, this will all become much easier for both the cli and pylibjuju as they just need to implement a simple challenge/response protocol and the controller will do all the grunt work.

[cderici]

Changes, along with their passing tests are posted on juju/python-libjuju#566 for all the three points:

1. Juju deploy allows you to specify a base bundle (+optional overlay(s) if you use the multi-doc syntax) plus any number of explicitly defined overlay files (e.g. juju deploy ./multidoc.yaml --overlay foo.yaml --overlay bar.yaml). You can technically concatenate them client-side before calling the facade (just add them as extra docs after the base). However, (there is a tricky bit here:

See the test named test_deploy_bundle_with_multiple_overlays_with_include_files in the tests file. Corresponding yamls for the overlays are posted as well.

2. Bundles support an "include" directive (search for include-file here). The include path is relative to the overlay location which is why you see this mechanism with multiple bundle data sources. Since the facade logic cannot report back errors like "give me file X" you will need to emulate this logic and expand include-{file,base64} directives client-side where you can access the files.

See the tests named test_deploy_local_bundle_include_file and test_deploy_local_bundle_include_base64 in the tests file.

3. Bundles/overlays can reference local charms (again, paths are relative to the thing that references them). I am not 100% sure how this can be made to work easily with the python client.

Turns out this is already working in pylibjuju, see this test, where the local.yaml file contents is:

applications:
  test1:
    charm: "../charm"
    num_units: 1
  test2:
    charm: "../charm.charm"
    num_units: 1

So as you suggested, include-file and include-base64 directives are being handled in the client-side 👍

[cderici]

deploy bundle with overlays

[SimonRichardson]

We need to bump the API. Current implementations might break if they see a multipart yaml doc and we don't want to do that. So the change is good if we add a new API revision.

[cderici]

We need to bump the API. Current implementations might break if they see a multipart yaml doc and we don't want to do that. So the change is good if we add a new API revision.

I don't think I follow, the only change this has is for GetChanges to take into account all the parts of a multipart yaml when returning a set of Changes, rather than the current behaviour of processing only the first part and ignoring the rest. So whoever's currently using it with a single part yaml will still be able to do that. And whoever wants to use it with a multipart yaml will still get a set of Changes. I guess I don't know what exactly this breaks.

[SimonRichardson]

Almost there. The facades are bumped correctly, we just need to wire up the different function calls depending on the version of the API.

[SimonRichardson]

So for the v1-v5 of the api, we want to call the old getBundleChanges. For the v6 of the api we want to call this new method.

[hmlanigan]

Nit: Please rename to TestGetChangesSuccessWithOverlays. V2 is confusing as V5 fails with overlays.

[cderici]

I'm not entirely sure why I named it V2 tbh, must be a copy paste overlook, good catch!

[hmlanigan]

The test fails when: apiv5 := &bundle.APIv5{api}. Looks like we're calling the wrong getChanges(). Will need a test for v1, v5 and the most recent version.

[hmlanigan]

Please make this an explicit check for content. Not here could be almost empty.

[cderici]

@hmlanigan thanks for pointing out the issues! Hope I was able to make things a bit clearer:

  $ cd juju/apiserver/facades/client/bundle/
  $ go test -gocheck.f TestGetChangesWithOverlaysV6
  $ go test -gocheck.f TestGetChangesWithOverlaysV1_5

[SimonRichardson]

I'm approving to unlock the request changes blocker.

I haven't Q&A'd this yet though.

[achilleasa]

This looks like fun...

[achilleasa]

Having to duplicate all this code makes it really hard to track what calls what.

How about we change the b.getChanges signature to include a boolean flag (or alternatively, a typed enum for more clarity) which indicates the parse mode (e.g. includeOverlays) to use?

Then you can extract the common GetChanges logic into a private method which accepts this flag and just provide two tiny stubs for APIV5 and BundleAPI which pass the right value through.

[cderici]

I agree, the duplication adds more indirection on the control flow. I initially tried to duplicate only the b.getChanges but it didn't work for reasons related to Go semantics that I don't understand yet.

However, if I'm understanding your proposal correctly, I'm also doubtful that adding another mechanic to separate functionality between versions would make this less confusing. Especially when more version breaks are added in the future, this might potentially turn into (already existing) duplicate codes with big switches in some of the functions.

Also, code duplication is often really bad for like maintainance/debug etc, but this duplication is like grabbing a snapshot of the current version and moving on, so day 2 ops are still gonna be done on the func (b *BundleAPI) GetChanges (unless it's specifically about <=APIv5). On the other hand, adding anohter argument would sort of cram all the logics into one spot, thereby forcing anyone maintaining/improving it in the future to understand all of it and make sure they're not breaking any other version logic.

[achilleasa]

My suggestion was something along these lines:

func (b *APIv5) GetChanges(args params.BundleChangesParams) (params.BundleChangesResults, error) {
 return b.doGetChanges(args, false)
}

// Versions 6+
func (b *BundleAPI) GetChanges(args params.BundleChangesParams) (params.BundleChangesResults, error) {
 return b.doGetChanges(args, true)
}

// You could also pass validators as an arg and use the same method for pre V5 facades
func (b *BundleAPI) doGetChanges(args params.BundleChangesParams, expandOverlays bool) (params.BundleChangesResults, error) {
    vs := validators{
		verifyConstraints: func(s string) error {
			_, err := constraints.Parse(s)
			return err
		},
		verifyStorage: func(s string) error {
			_, err := storage.ParseConstraints(s)
			return err
		},
		verifyDevices: func(s string) error {
			_, err := devices.ParseConstraints(s)
			return err
		},
	}
	return b.getBundleChanges(args, vs, expandOverlays, mapBundleChanges)
}

func mapBundleChanges(changes []bundlechanges.Change, results *params.BundleChangesResults) error {
  results.Changes = make([]*params.BundleChange, len(changes))
  for i, c := range changes {
	  var guiArgs []interface{}
	  switch c := c.(type) {
	  case *bundlechanges.AddApplicationChange:
		  guiArgs = c.GUIArgsWithDevices()
	  default:
		  guiArgs = c.GUIArgs()
	  }
	  results.Changes[i] = &params.BundleChange{
		  Id:       c.Id(),
		  Method:   c.Method(),
		  Args:     guiArgs,
		  Requires: c.Requires(),
	  }
  }
  return nil
}

func (b *BundleAPI) getBundleChanges(...){
  // merge getChanges + getBundleChanges logic
}

The way that the facade abuses inheritance should allow this to work but as you can see the code is much more compact. Also, note that when we move to server-side bundle expansion this facade will become obsolete as we will be using the same code path for both the juju cli and pylibjuju and the controller will take care of calculating and applying the required changes.

[cderici]

@achilleasa Ah ok I see what you're saying, I agree, this looks much better, I'll see what I can do, thanks for the input 👍

[achilleasa]

Forgot to add this handy snippet if you want to add some more context instead of passing true/false:

const (
   ignoreOverlays = false
   includeOverlays = true
)

...

func (b *APIv5) GetChanges(args params.BundleChangesParams) (params.BundleChangesResults, error) {
 return b.doGetChanges(args, ignoreOverlays)
}

// Versions 6+
func (b *BundleAPI) GetChanges(args params.BundleChangesParams) (params.BundleChangesResults, error) {
 return b.doGetChanges(args, includeOverlays)
}

[achilleasa]

This line can be omitted as the default zero value is nil.

[achilleasa]

I would recommend extracting this, and the v5 below (L270) to standalone functions to improve readability.

[achilleasa]

Thanks for applying the extra changes to clean up the code. Happy to see this landed if CI is green.

[cderici]

$$merge$$

[juanmanuel-tirado]

JUJU-142