WIP: RFC: pkg/assets: Merkle DAG asset library

[wking]

Our current pkg/asset approach requires multiple stages to run, and selecting those stages requires some dev planning and user education. This new package allows us to dump the whole asset graph at once, and then edit/rebuild multiple times while maintaining a consistent asset state. I think this will be easier for folks to wrap their heads around, but I'm not always the best judge of approachability ;). Check out the updated user docs (included in the PR) for an overview of the new approach.

If folks think this approach is worthwhile, I can keep working through the process of porting our old assets over to the new framework. Currently there are only a handful as a proof-of-concept.

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wking

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 [wking]

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

[abhinavdahiya]

went through this as first pass:

• this adds complexity without giving us any more features.. so i'm skeptical
• all the files for user to see is very jaaring and prone to all users making wrong choices
• not big fans of all the init() functions
• forces all these assets to only depend on data []byte for each other, i like types.
• not sure how assets can define custom logic for read...
• bf48805#diff-b652ef1d0f4325b91400fd6734abd124R138 this seems like a foot gun as user can change both this asset and its dependency as they are disk but it looks like we just pick this asset as is and the parent (its dependecy) changes are neglected???

will keep looking. i'm a little unsure if we need this complexity...

[wking]

• this adds complexity without giving us any more features.. so i'm skeptical
• all the files for user to see is very jaaring and prone to all users making wrong choices

I agree that there will be lots of files. To mitigate that, we could use .{name} for the JSON (or put them in a separate sub-directoy), since most users won't care about them.

But to balance the "this is a lot of files":

• There's a dependency graph. And besides the mostly-complete one you'll be able to find in the docs, you can now run graph against an asset directory to see your dependencies.
• The files don't need to go anywhere between clusters. Once you're happy with create assets, you'll be able to cycle between create cluster and destroy cluster without needing to return to create assets. I think this will reduce the chances of users doing things like` removing their state while leaving a branch asset like their kubeconf around ("openshift-install destroy cluster" leaves auth dir, breaking next install  #522, README: Add more hints about cleanup and reinstallation #532).
• I think there's simplicity in dropping the target and writeable-asset distinctions. Now there is only one rendering command and only one type of asset.

• not big fans of all the init() functions

This allows me to colocate the rebuilder/defaulter definitions with their registrations. If you'd rather have me centralize the registrations in one big, non-init var, I could do that. But note that ConstantDefault registries for some scripts and templates will be spacious.

• forces all these assets to only depend on data []byte for each other, i like types.

It's all bytes on disk for both this and our current approach. If you want helpers to unpack a particular asset into a struct for its children, I can certainly add helpers like this as needed.

• not sure how assets can define custom logic for read...

That's a bonus, right? Why would you want custom read logic? ;)

• bf48805 #diff-b652ef1d0f4325b91400fd6734abd124R138 this seems like a foot gun as user can change both this asset and its dependency as they are disk but it looks like we just pick this asset as is and the parent (its dependecy) changes are neglected???

We ignore future changes to its ancestors, we continue to propagate down to its decendants. If we wanted, we could record its parent references when we freeze it and log warnings if those parents changed (e.g. "you've added a custom kubeconfig, but its previous parent root-ca.crt has changed from old-hash to new-hash", etc.).

[abhinavdahiya]

There's a dependency graph. And besides the mostly-complete one you'll be able to find in the docs, you can now run graph against an asset directory to see your dependencies.

:/

The files don't need to go anywhere between clusters. Once you're happy with create assets, you'll be able to cycle between create cluster and destroy cluster without needing to return to create assets. I think this will reduce the chances of users doing things like` removing their state while leaving a branch asset like their kubeconf around (#522, #532).

so this requires users keep the whole dir around for that, i don't see how that is differnet that state file.
#547 shows current approach can be successfull in same action.

I think there's simplicity in dropping the target and writeable-asset distinctions. Now there is only one rendering command and only one type of asset.

I personally have been trying my best to keep us from adding new interfaces too, but targets are a better UX

This allows me to colocate the rebuilder/defaulter definitions with their registrations. If you'd rather have me centralize the registrations in one big, non-init var, I could do that. But note that ConstantDefault registries for some scripts and templates will be spacious.

The current impl is able to work without any init

It's all bytes on disk for both this and our current approach. If you want helpers to unpack a particular asset into a struct for its children

Current impl is slowly moving away from bytes... https://github.com/openshift/installer/blob/master/pkg/asset/machines/worker.go#L69 is accessing the installconfig type directly, not marshal/unmarshalling, while this forces us to go back

That's a bonus, right? Why would you want custom read logic? ;)

reading expired certs from disk etc validations can be useful to user.

We ignore future changes to its ancestors, we continue to propagate down to its decendants. If we wanted, we could record its parent references when we freeze it and log warnings if those parents changed (e.g. "you've added a custom kubeconfig, but its previous parent root-ca.crt has changed from old-hash to new-hash", etc.).

The current impl can handle that case easily too, disregard parents when asset from disk is read and modified...

Also how do we model a case where user wants to bring new files to an asset, eg. manfiests from current impl.

[wking]

I've pushed dd9b8cc -> 20912ea, rebasing onto master and adding enough assets to generate a reasonable graph.

The files don't need to go anywhere between clusters. Once you're happy with create assets, you'll be able to cycle between create cluster and destroy cluster without needing to return to create assets. I think this will reduce the chances of users doing things like` removing their state while leaving a branch asset like their kubeconf around (#522, #532).

so this requires users keep the whole dir around for that, i don't see how that is differnet that state file.
#547 shows current approach can be successfull in same action.

Yeah, #547 will give us similar behaviour here. The difference is that with #547 you still need to find the right target to access the resource(s) you want to manipulate, while with this approach you (re)generate all the assets at once.

I think there's simplicity in dropping the target and writeable-asset distinctions. Now there is only one rendering command and only one type of asset.

I personally have been trying my best to keep us from adding new interfaces too, but targets are a better UX

Because...? It it just that there's less to think about at each stage? One extension I was thinking about was:

openshift-install [options] create assets [TARGET ...]

to allow you to create subsets of the graph. For example, you could:

openshift-install --dir example create assets bootstrap.ign

to just create assets through to the bootstrap.ign target. A subsequent ... create assets call would fill in the rest of the graph. This would allow users who wanted separate stages to recover that behavior for a single intermediate step. There wouldn't be any of the current master's removing of "consumed" asset files, so subsequent explicit targets would still have asset files from the previous iteration.

This allows me to colocate the rebuilder/defaulter definitions with their registrations. If you'd rather have me centralize the registrations in one big, non-init var, I could do that. But note that ConstantDefault registries for some scripts and templates will be spacious.

The current impl is able to work without any init

Because the current implementation requires consumers to explicitly import their parents (e.g. here). With this PR, you just feed your parents' names into getByName (e.g. here). Not requiring a separate dependency lister allows for dynamic dependencies (e.g. you can pull the platform asset, see that it is libvirt and skip your potential AWS and OpenShift parents). But you could still get dynamic dependencies by collecting all the registration in a single function, or in per-package functions (e.g. tlsDefaults() map[string]Defaulter and tlsRebuilders() map[string]assets.Rebuild if your issue is just with the use of init.

It's all bytes on disk for both this and our current approach. If you want helpers to unpack a particular asset into a struct for its children

Current impl is slowly moving away from bytes... https://github.com/openshift/installer/blob/master/pkg/asset/machines/worker.go#L69 is accessing the installconfig type directly, not marshal/unmarshalling, while this forces us to go back

It's still unmarshalling when we pull it up off the disk. The savings are just efficiency for assets that are consumed in multiple places. I don't think we have so many of those that unmarshalling efficiency is going to be a big deal, but I can run some benchmarks if you want once I get the asset tree filled in a bit more.

That's a bonus, right? Why would you want custom read logic? ;)

reading expired certs from disk etc validations can be useful to user.

Validation sounds useful. Adding a new Asset.Validate property could address that, no?

We ignore future changes to its ancestors, we continue to propagate down to its decendants. If we wanted, we could record its parent references when we freeze it and log warnings if those parents changed (e.g. "you've added a custom kubeconfig, but its previous parent root-ca.crt has changed from old-hash to new-hash", etc.).

The current impl can handle that case easily too, disregard parents when asset from disk is read and modified...

Right, currently it's disregarding the from-disk asset when a parent changes, but we could change that. Looking through master's store.go though, I don't see any dirty flag being persisted in the state (like I'm persisting frozen ). Am I missing something? What happens if you want to drop in a custom worker ignition (say) and have it persist across multiple cluster launches? Or, with the current coarse-grained assets in master, is there a way to persist your custom kube-apiserver-config-overrides.yaml across multiple cluster launches?

The current asset approach requires a lot of Go overhead for each asset, while the approach I have here requires very little. For example, compare this 29-line file for the kube CA with the current 44 liner. And I have the bootstrap units a separate assets injected with one line each, while the current master pushes them into the bootstrap asset, presumthoably because the overhead of supporting Dependencies, Generate, Name, Files, and Load for each asset is too much trouble.

Also how do we model a case where user wants to bring new files to an asset, eg. manfiests from current impl.

Yeah, there's not an elegant way to do that. We could hack something in for manifests in particular (do we need this for other asset types beyond manifests?). As this branch stands, the easiest approach is probably stuffing your manifest into bootstrap.ign.data via jq or similar after each asset rebuild (which isn't bad). But you'd need to remember to remove bootstrap.ign before each rebuild to clear your freeze and pick up parent changes.

[wking]

I've pushed 20912ea -> 0222815 expanding the graph a bit more and changing {slug} and {slug}.data to .state/{slug} and {slug} respectively. That makes the editable raw-data files more visible and makes the JSON-state files less visible. It may also mitigate @abhinavdahiya's concerns of the size of the asset directory, although there are still going to be a lot of files in there.

One possibility to balance my interest in (re)rendering as a single step with @abhinavdahiya's concerns about overwhelming users would be to write these files into subdirectories (e.g. tls/... for all the TLS assets, manifests/... for all the manifests, etc.). Thoughts?

[wking]

I've pushed 0222815 -> 49efd2d (new graph) implementing asset subdirectories. The generated asset state (still very incomplete) now looks like:

$ openshift-install --dir=wking create assets --prune
$ tree wking
wking
├── auth
│   ├── kubeconfig-admin
│   └── kubeconfig-kubelet
├── base-domain
├── bin
│   ├── bootkube.sh
│   └── report-progress.sh
├── cluster
├── cluster-name
├── ignition
│   └── bootstrap.ign
├── ignition-configs
├── manifests
│   ├── cluster-apiserver-certs.yaml
│   └── kube-apiserver-secret.yaml
├── overrides
│   ├── kube-apiserver-config-overrides.yaml
│   └── kube-controller-manager-config-overrides.yaml
├── ssh.pub
├── tls
│   ├── admin-client.crt
│   ├── admin-client.key
│   ├── aggregator-ca.crt
│   ├── aggregator-ca.key
│   ├── cluster-apiserver-ca.crt
│   ├── cluster-apiserver-ca.key
│   ├── kube-ca.crt
│   ├── kube-ca.key
│   ├── kubelet-client.crt
│   ├── kubelet-client.key
│   ├── root-ca.crt
│   └── root-ca.key
└── unit
    ├── bootkube.service
    ├── kubelet.service
    ├── progress.service
    └── tectonic.service

7 directories, 30 files

Folks interested in install-config level settings can just look at:

$ ls -F --group-directories-first wking | cat
auth/
bin/
ignition/
manifests/
overrides/
tls/
unit/
base-domain
cluster
cluster-name
ignition-configs
ssh.pub

[abhinavdahiya]

this is open to duplications?

[wking]

this is open to duplications?

It is. I'm comfortable avoiding that by not requesting the same parent twice from a single rebuilder. But if you'd prefer a guard to catch folks who do so by mistake, or if you'd like me to allow repeat lookups without duplicate additions, I can put either of those in.

[abhinavdahiya]

58b31c2 I don't think this is the best way to allow any kind of review. i can't compare or verify if your copied asset impl is correct. please make changes inplace.

also see the dev docs on asset generation were dropped, i think they were useful as they allow any body new to look and add new asset.

Also there is no mention of Defaults vs Rebuilder that you have registered, why, when, or to use them?

[wking]

58b31c2 I don't think this is the best way to allow any kind of review. i can't compare or verify if your copied asset impl is correct. please make changes inplace.

I could squash things down, but to get an in-place pivot, I'd need to squash all of:

* 58b31c2 pkg/asset: Remove now that we have pkg/installerassets
* e2ca6ea cmd/openshift: Transition to the new asset graph
* f646d01 pkg/installerassets: Use the generic asset package

together (and possibly also the doc updates currently in 813ea6e). I can do that, you can see pretty much the same diff via GitHub's file view, and I think that in general I'm mutating files too much for Git to be able to see things like the pkg/asset/tls/root.go -> pkg/installerassets/tls/root.go.

Or are you saying I should rename my files to preserve the old filenames where possible? I could do that, but I'd like to keep the generic pkg/assets (59b9fa6) separate from our installer-specific assets (which I'm currently adding in pkg/installerassets). And having both package/assets and package/asset side by side seems confusing ;). Maybe you want package/assetframework or some such with the generic stuff, and pkg/asset for the installer-specific stuff?

also see the dev docs on asset generation were dropped, i think they were useful as they allow any body new to look and add new asset.

The user-level portion of this doc moved into docs/user/overview.md. I thought pkg/installerassets gave enough examples of injecting assets for it to not need additional docs, but I can add them if you want (by fleshing out these with some examples and associated patter?

Also there is no mention of Defaults vs Rebuilder that you have registered, why, when, or to use them?

Because users don't need to care about them, and devs can read the (currently sparse) godocs. I'll take a stab at extending the godocs, and we'll see how that feels to you.

[wking]

I've rebased onto master with 49efd2d -> 5994f8b (new graph). The graph nodes are now colored by directory to make it easier to get a quick overview. I've also added more docs to help orient devs:

$ godoc ./pkg/installerassets
...
VARIABLES

var Defaults = make(map[string]Defaulter)
    Defaults registers installer asset default functions by name. Use this
    to set up assets that do not have parents. For example, constants:

	Defaults["your/asset"] = ConstantDefault([]byte("your value"))

    or values populated from outside the asset graph:

	Defaults["your/asset"] = func() (data []byte, err error) {
	  value = os.Getenv("YOUR_ENVIRONMENT_VARIABLE")
	  return []byte(value), nil
	}

var Rebuilders = make(map[string]assets.Rebuild)
    Rebuilders registers installer asset rebuilders by name. Use this to set
    up assets that have parents. For example:

	func yourRebuilder(getByName assets.GetByString) (asset *assets.Asset, err error) {
	  asset = &assets.Asset{
	    Name:          "tls/root-ca.crt",
	    RebuildHelper: rootCARebuilder,
	  }

	  parents, err := asset.GetParents(getByName, "tls/root-ca.key")
	  if err != nil {
	    return nil, err
	  }

	  // Assemble your data based on the parent content using your custom logic.
	  for name, parent := range parents {
	    asset.Data = append(asset.Data, parent.Data)
	  }

	  return asset, nil
	}

    and then somewhere (e.g. an init() function), add it to the registry:

	Rebuilders["your/asset"] = yourRebuilder
...

[openshift-ci-robot]

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

[openshift-ci-robot]

@wking: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-libvirt	38bff8c	link	/test e2e-libvirt
ci/prow/e2e-aws	38bff8c	link	/test e2e-aws
ci/prow/rhel-images	38bff8c	link	/test rhel-images
ci/prow/e2e-aws-upgrade	38bff8c	link	/test e2e-aws-upgrade
ci/prow/verify-vendor	38bff8c	link	/test verify-vendor

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[wking]

I still like this approach, but I'm giving up on rebasing it ;).

/close

[openshift-ci-robot]

@wking: Closed this PR.

In response to this:

I still like this approach, but I'm giving up on rebasing it ;).

/close

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.