-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create asset graph engine #120
Create asset graph engine #120
Conversation
installer/pkg/nextgen/asset.go
Outdated
|
||
type Asset interface { | ||
Dependencies() []Asset | ||
Generate([]*AssetState) (*AssetState, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would nice if we Generate could take entire State
https://github.com/openshift/installer/blob/master/Documentation/design/assetgeneration.md#state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be nice about getting the entire State
? The asset has already stated what its dependencies are. If it needs anything from State
beyond those dependencies, then it is using a dependencies that it did not declare. The AssetStore
has already fetched all of the states for all of the dependencies, so there is no need for the asset to relocate them from the entire State
.
installer/pkg/nextgen/asset/graph.go
Outdated
@@ -0,0 +1,35 @@ | |||
package asset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should try to avoid defining this graph defining.
return nil, fmt.Errorf("unknown platform type %q", platform) | ||
} | ||
|
||
json, err := jsoniter.ConfigFastest.Marshal(installConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why specfically marshal to json then, marshal to yaml is required. "github.com/ghodss/yaml".Marshal() already does it?
installer/pkg/nextgen/asset/graph.go
Outdated
|
||
type Graph struct { | ||
// Targetable assets | ||
InstallConfig nextgen.Asset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this specialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Targetable assets vs Non-targetable assets
installer/pkg/nextgen/asset/graph.go
Outdated
// Targetable assets | ||
InstallConfig nextgen.Asset | ||
|
||
// Non-targetable assets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a targetable/non-targetable distinction? Why couldn't the password asset, etc. correspond to on-disk files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need the distinction. There is no code that differentiates a targetable asset from a non-targetable asset. This is just a helpful delineation to make clear which asset are the ones that the user would directly generate as opposed to being indirectly generated by other assets.
I don't see why the password asset could not correspond to on-disk files.
installer/pkg/nextgen/asset.go
Outdated
package nextgen | ||
|
||
type Asset interface { | ||
Dependencies() []Asset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to @abhinavdahiya's comment, this should return a slice of parent hashes, not the parent assets themselves. That makes it a bit easier to get the asset's Merkle hash, and you would Fetch
the parent by hash if you needed more detail about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you explain a little more how the assetstore might use the hash from Dependencies to actually generate the corresponding asset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependencies() defines the assets it requires. asset store ensures that those assets are in state before calling generate on the it asset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed the discussion related to using a Merkle DAG for the asset store. Could someone elaborate on how we plan on using the properties of a Merkle DAG? Maybe this could be added to the design document.
installer/pkg/nextgen/assetstore.go
Outdated
package nextgen | ||
|
||
type AssetStore interface { | ||
Fetch(Asset) (*AssetState, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Maybe I'm biased by Git, but for me "fetch" hints of remote access. Can we use the more generic Get
? And adjust this to be content-addressable retrieval:
Get(hash string) (Asset, error)
I also think the store needs a property for the root hash (or maybe a slice of root hashes?) so we can walk the current DAG rebuilding/extracting assets without tripping over stale entries.
LibvirtPlatformType = "libvirt" | ||
) | ||
|
||
type Platform struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like the platform asset should be an instance of UserProvided
instead of a type in its own right. Maybe add a Validate
property holding a validation function to UserProvided
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it much matters whether it is one type or two separate types. In the end, I decided on having Platform as a separate type because there was extra code that needed to go somewhere to define the Validate for the platform and to define the acceptable platforms. Either I put this extra code in stock.go, which will tend to make that file grow too large, or I create a separate file for the extra platform code. If I create a separate file, then I may as well create a separate type, too, which has the extra benefit of keeping the UserProvided type smaller.
installer/cmd/nextgen/main.go
Outdated
@@ -0,0 +1,50 @@ | |||
package main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put it under just ${installer_root}/pkg
?
I was expecting something like:
openshift-installer/
├── cmd
│ └── openshift-install
└── pkg
└── graph(or asset?)
We should just create a greenfield for the new installer binary.
installer/pkg/nextgen/asset/graph.go
Outdated
g.License = &UserProvided{Prompt: "License: "} | ||
g.PullSecret = &UserProvided{Prompt: "Pull Secret: "} | ||
g.Platform = &UserProvided{Prompt: "Platform: "} | ||
g.EmailAddress = &UserProvided{Prompt: "Email Addres: "} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Email Addres/Email Address
} | ||
|
||
func (a *InstallConfig) Generate(dependencies []*nextgen.AssetState) (*nextgen.AssetState, error) { | ||
//emailAddress := string(dependencies[0].Contents[0].Data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should pass a map?
@staebler lgtm so far |
pkg/types/installconfig.go
Outdated
metav1.ObjectMeta `json:"metadata"` | ||
|
||
// ClusterID is the ID of the cluster. | ||
ClusterID uuid.UUID `json:"clusterID"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather have this be a []byte
(or hex-encoded string
?). Is there a reason to preserve the UUID
type in this config? Using a UUID algorithm to generate a unique ID makes sense, but once it's generated I think we can treat it as an opaque ID and won't need access to any UUID
-specific methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the design (https://github.com/openshift/installer/blame/master/Documentation/design/installconfig.md#L246), it is specified as a uuid.UUID
, so that is what I used. I agree that we can likely treat is as an opaque ID, but in the short-term I will leave it as a uuid.UUID
to match the design.
cmd/openshift-installer/main.go
Outdated
log "github.com/Sirupsen/logrus" | ||
"gopkg.in/alecthomas/kingpin.v2" | ||
|
||
"github.com/openshift/installer/pkg/asset" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like these new package paths, avoiding our current github.com/openshift/installer/installer/...
redundancy. But to get CI tests for these new locations, we'll need updates to go-lint.sh
, go-vet.sh
, ci-operator (also here), etc. I'm not sure about the best way to roll those out. Maybe something like:
- Land an installer PR with a stub
cmd/openshift-installer/main.go
andpkg/
directory with the associated local CI updates. - Land a release PR with updates to add tests for the new locations.
- Come back and rebase this PR around the stubs from step 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it be OK to add the CI updates after this PR lands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it be OK to add the CI updates after this PR lands?
I've filed openshift/release#1289 bumping unit and go-lint to cover the new directories. And I've filed #185 with the local hack/
changes.
cmd/openshift-installer/main.go
Outdated
@@ -0,0 +1,49 @@ | |||
package main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe just rename this folder to cmd/openshift-install/main.go
,
So the binary will be openshift-install
.
Then we can say things like
openshift-install installconfig
openshift-install manifests
openshift-install ignconfigs
openshift-install cluster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we can say things like
openshift-install installconfig openshift-install manifests
👍 to cmd/openshift-install
, but I don't see why we'd want sub-commands for subsets of the graph. Why not generate
to (re)generate all the assets, install
to push them out, and destroy
to tear down?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking The sub-commands are used so that the user has an opportunity to make manual modifications to the generated assets from the previous step before they are used by subsequent steps.
pkg/asset/userprovided.go
Outdated
for { | ||
fmt.Print(prompt) | ||
var input string | ||
if _, err := fmt.Scanln(&input); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like Scanln
is breaking on whitespace (regardless of newline-ness). I think we may need to use a Scanner.Scan
(although there are other options as well). Here's a play showing the issue (note the "expected newline" error on the first Fscanln
).
For most of our inputs (email addresses, domain names, ...), we don't want spaces in the input. But the pull secret is JSON, which may contain spaces. And folks could have spaces in their password(/phrase). So where we want to enforce "this line contains no spaces" (or "this line contains an @
and other valid email-address structure"), I'd rather leave that to explicit validation, instead of having it be a limit imposed by the generic queryUser
.
pkg/asset/stock.go
Outdated
"os" | ||
) | ||
|
||
// Stock is the stock of assets that cen be generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo "cen" -> "can"
pkg/asset/userprovided.go
Outdated
input = input[:len(input)-1] | ||
} | ||
if a.validation != nil { | ||
validatedInput, ok := a.validation(input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine returning a cleaned version of the raw input (your validatedInput
), but I think the UX would be better if you returned an error
with a message instead of the boolean ok
. One benefit of that approach is that you can use a number of our existing validators (with a wrapper if you keep validatedInput
). For example, see the ValidateInput
type in my mock-up, used here and configured here.
@yifan-gu There are few other missing fields, |
- stretchr/testify - pborman/uuid
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: staebler, yifan-gu 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:
Approvers can indicate their approval by writing |
/lint ^ this should run checks on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking: 12 warnings.
In response to this:
/lint
^ this should run checks on
pkg/...
as well, since we're currently not covering those in ourgolint
Prow job.
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.
@@ -0,0 +1,36 @@ | |||
package asset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint comments: should have a package comment, unless it's in another file for this package. More info.
@@ -0,0 +1,122 @@ | |||
package types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint comments: should have a package comment, unless it's in another file for this package. More info.
@@ -0,0 +1,40 @@ | |||
package asset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint comments: should have a package comment, unless it's in another file for this package. More info.
@@ -0,0 +1,100 @@ | |||
package installconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint comments: should have a package comment, unless it's in another file for this package. More info.
@@ -0,0 +1,46 @@ | |||
package asset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint comments: should have a package comment, unless it's in another file for this package. More info.
@@ -0,0 +1,98 @@ | |||
package installconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint comments: should have a package comment, unless it's in another file for this package. More info.
@@ -0,0 +1,44 @@ | |||
package main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint comments: should have a package comment, unless it's in another file for this package. More info.
@@ -0,0 +1,55 @@ | |||
package types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint comments: should have a package comment, unless it's in another file for this package. More info.
@@ -0,0 +1,27 @@ | |||
package stock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint comments: should have a package comment, unless it's in another file for this package. More info.
@@ -0,0 +1,125 @@ | |||
package installconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint comments: should have a package comment, unless it's in another file for this package. More info.
/test e2e-aws |
The e2e-aws error was:
That's a flake we see occasionally (e.g. here). Retesting would probably complete without this error, but since the error removes us from the merge queue, we may want to take the opportunity to address the golint issues. |
@wking I think it's fine, I can take care of the golint warning in my PR. |
} | ||
|
||
states := map[asset.Asset]*asset.State{ | ||
stock.clusterID: &asset.State{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently don't run gofmt
in CI (I've got work in flight to add that, #173), but running it locally complains about this line and the later ones following the same pattern:
$ git describe --always --abbrev=0
1273983432bf1ee9dbf51cd71cf8aaf7a6be798b
$ find . -name '*.go' ! -path '*/vendor/*' ! -path '*/.build/*' -exec gofmt -s -w {} \+
$ git diff --exit-code pkg
diff --git a/pkg/asset/installconfig/installconfig_test.go b/pkg/asset/installconfig/installconfig_test.go
index 32fcb58..0355717 100644
--- a/pkg/asset/installconfig/installconfig_test.go
+++ b/pkg/asset/installconfig/installconfig_test.go
@@ -120,28 +120,28 @@ func TestInstallConfigGenerate(t *testing.T) {
}
states := map[asset.Asset]*asset.State{
- stock.clusterID: &asset.State{
+ stock.clusterID: {
Contents: []asset.Content{{Data: []byte("test-cluster-id")}},
},
- stock.emailAddress: &asset.State{
+ stock.emailAddress: {
Contents: []asset.Content{{Data: []byte("test-email")}},
},
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed via #181.
The e2e-aws error was resource exhaustion:
I'm cleaning up the CI account with my v3 script now. |
We're down to one pending e2e-aws job, so there should be plenty of resources free. /retest |
Broke by openshift/release#1274 landing before #168 :/ |
/test e2e-aws |
openshift/release#1284 has reverted openshift/release#1274 until we get #168 landed. |
Catching up with 4e92db8 (Merge pull request openshift#120 from staebler/asset_graph_engine, 2018-08-24). * Drop the go-lint comment from go-fmt.sh. I'd accidentally copy/pasted this over in 87b3e17 (hack/go-fmt: Make it easy to auto-format Go, 2018-08-24, openshift#173). * Suggest users run go-lint over pkg/... as well. * Simplify the 'go vet' invocation. The code I'm removing is from eb71c8d (Added go-vet shell script, 2018-08-06, openshift#110). The Travis config it replaced was removed in 1765e93 (.travis.yml: Prune moved-to-Prow tf-lint, etc., 2018-08-14, openshift#123), but was just running: go vet ./installer/... inside the container. So I don't think we need the directory changing or moving here. And we should certainly be able to cover the test from a single container, instead of launch a new container for each requested directory (at least as long as the requested directories are under $PWD, and the old script required that anyway).
Cover ./pkg/... (and, for go-lint, ./cmd/...). These directories are new with openshift/installer@4e92db8 (Merge pull request openshift#120 from staebler/asset_graph_engine, 2018-08-24, openshift/installer#120). There's no particular reason to not run the Go unit tests on ./cmd/... But we're unlikely to have unit tests there, and the old tests restricted themselves to ./installer/pkg/..., so I've stuck with that pattern here. Steve recommended using 'go list ...' to run the lint commands [1]. We just want something that will give us our Go directories without dropping down into the vendor directories. [1]: openshift#1289 (comment)
We've had the dependency since 989532b (Add some dependencies used for asset generation, 2018-08-10, openshift#120), so using it here doesn't cost anything more than we're already paying ;). And it's nice to have compact diffs like: Diff: --- Expected +++ Actual @@ -33,8 +33,4 @@ networking: - podCIDR: - IP: 10.2.0.0 - Mask: //8AAA== - serviceCIDR: - IP: 10.3.0.0 - Mask: //8AAA== + podCIDR: 10.2.0.0/16 + serviceCIDR: 10.3.0.0/16 type: canal Test: TestKubeSystem instead of comparing long YAML docs by eye. The library does expect the expected value to be passed in before the actual value [1]. [1]: https://godoc.org/github.com/stretchr/testify/assert#Equal
…rmName* The old *PlatformType are from cccbb37 (Generate installation assets via a dependency graph, 2018-08-10, openshift#120), but since 476be07 (pkg/asset: use vendored cluster-api instead of go templates, 2018-10-30, openshift#573), we've had variables for the name strings in the more central pkg/types. With this commit, we drop the more peripheral forms. I've added a unit test to enforce sorting in PlatformNames, because the order is required by sort.SearchStrings in queryUserForPlatform.
…orm}.Name The old *PlatformType are from cccbb37 (Generate installation assets via a dependency graph, 2018-08-10, openshift#120), but since 476be07 (pkg/asset: use vendored cluster-api instead of go templates, 2018-10-30, openshift#573), we've had variables for the name strings in the more central pkg/types. With this commit, we drop the more peripheral forms. I've also pushed the types.PlatformName{Platform} variables down into types.{platform}.Name at Ahbinav's suggestion [1]. I've added a unit test to enforce sorting in PlatformNames, because the order is required by sort.SearchStrings in queryUserForPlatform. [1]: openshift#659 (comment)
…orm}.Name The old *PlatformType are from cccbb37 (Generate installation assets via a dependency graph, 2018-08-10, openshift#120), but since 476be07 (pkg/asset: use vendored cluster-api instead of go templates, 2018-10-30, openshift#573), we've had variables for the name strings in the more central pkg/types. With this commit, we drop the more peripheral forms. I've also pushed the types.PlatformName{Platform} variables down into types.{platform}.Name at Ahbinav's suggestion [1]. I've added a unit test to enforce sorting in PlatformNames, because the order is required by sort.SearchStrings in queryUserForPlatform. [1]: openshift#659 (comment)
…le.rhel-baseimages-to-mach-ocp-build-data-config Bug 1878163: Updating Dockerfile.rhel baseimages to mach ocp-build-data config
This PR establishes the initial framework for generating assets using a graph engine (https://jira.coreos.com/browse/CORS-758).
go build ./cmd/openshift-install
.asset.Store
type that generates an asset by walking the tree of dependencies for the asset ensuring that each asset of a dependency has been generated prior to generating the dependent asset.asset.Stock
type that establishes the available assets that can be generated../openshift-install install-config
. The user-provided assets as crude with respect to UX and still need further refinement. The install-config asset does not fully populate the install-config.yml yet.