Skip to content

Skip generation of dependencies for on-disk assets - #639

Merged
openshift-merge-robot merged 1 commit into
openshift:masterfrom
staebler:do_not_generate_unneeded_dependents
Nov 14, 2018
Merged

Skip generation of dependencies for on-disk assets#639
openshift-merge-robot merged 1 commit into
openshift:masterfrom
staebler:do_not_generate_unneeded_dependents

Conversation

@staebler

@staebler staebler commented Nov 8, 2018

Copy link
Copy Markdown
Contributor

This allows the user to supply and use an on-disk asset (such as install-config.yml) without
need to also supply the state file that was created. This is helpful when re-using an on-disk
asset for multiple installations. In particular, hive would like to run openshift-install
with a supplied install-config.yml and no state file.

To effect this behavior, the asset store loads all of the on-disk assets that a fetched asset
depends upon prior to fetching the dependencies for the fetched asset. From this, the asset
store can determine whether the fetched asset is dirty or not. If the fetched asset is not
dirty and is on-disk, then the on-disk asset is used as is without generating any of the
dependent assets--as they would be ignored in resolving the fetched asset anyway.

Conflicts can occur when the asset store resolves in different ways the fetch of two assets
that share a dependency. For example, let us say that there are two assets, A and B, that both
depend upon asset C. Asset A is present on disk, and asset B is not present on disk. When the
asset store fetchs asset A, then asset C will not be generated. However, when the asset store
fetches asset B, then asset C will be generated in order to generate asset B. Asset A could
potentially have data that conflicts with the data that would have been taken from the asset
C that was generated.

Fixes #545

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 8, 2018
Comment thread pkg/asset/.openshift_install_state.json Outdated
Comment thread pkg/asset/store.go Outdated
Comment thread pkg/asset/store.go Outdated
Comment thread pkg/asset/store.go Outdated
@staebler

staebler commented Nov 13, 2018

Copy link
Copy Markdown
Contributor Author

What about the case where an asset is dirty and is in the state file?

@crawford The behavior when an asset is dirty and is in the state file hasn't changed. If any of the parents of an asset are dirty, then the asset is generated, regardless of whether the asset is in the state file. If the asset is dirty by virtue of being on-disk but none of its parents are dirty, then the on-disk source is used, regardless of whether the asset is in the state file.

Comment thread pkg/asset/store.go Outdated
Comment thread pkg/asset/store.go Outdated
@abhinavdahiya

abhinavdahiya commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

@staebler i think we can improve the loadfromdisk to take some of the decisions around disk source and state source. should make fetch simpler. WDYT?

@staebler

Copy link
Copy Markdown
Contributor Author

@staebler i think we can improve the loadfromdisk to take some of the decisions around disk source and state source. should make fetch simpler. WDYT?

@abhinavdahiya Yeah, I've had that lingering thought in the back of my head. The loadFromDisk function already does some work with the state file. I'll see what I can do about moving more of the work there.

Comment thread pkg/asset/store.go Outdated
@abhinavdahiya

Copy link
Copy Markdown
Contributor

8c174a8 we shouldn't need the global variables, but i think we can revisit testing later on...

@staebler

Copy link
Copy Markdown
Contributor Author

8c174a8 we shouldn't need the global variables, but i think we can revisit testing later on...

@abhinavdahiya Perhaps I'm just too in the weeds to see how it can be done without defining a type for each individual test case.

Comment thread pkg/asset/store.go Outdated
@abhinavdahiya

Copy link
Copy Markdown
Contributor

this looks good. can you squash the commits to more appropriate number so that its merge ready.

@staebler

Copy link
Copy Markdown
Contributor Author

this looks good. can you squash the commits to more appropriate number so that its merge ready.

@abhinavdahiya Your last review comment has been addressed. And the commits have been squashed.

…e of its dependencies are dirty

This allows the user to supply and use an on-disk asset (such as
install-config.yml) without the need to also supply the state file that was
created. This is helpful when re-using an on-disk asset for multiple
installations. In particular, hive would like to run openshift-install
with a supplied install-config.yml and no state file.

To effect this behavior, the asset store loads all of the on-disk assets that a
fetched asset depends upon prior to fetching the dependencies for the fetched
asset. From this, the asset store can determine whether the fetched asset is
dirty or not. If the fetched asset is not dirty and is on-disk or in the state
file, then the asset is used as is without generating any of the dependent
assets--as they would be ignored in resolving the fetched asset anyway.

Conflicts can occur when the asset store resolves in different ways the fetch of
two assets that share a dependency. For example, let us say that there are two
assets, A and B, that both depend upon asset C. Asset A is present on disk, and
asset B is not present on disk. When the asset store fetchs asset A, then asset
C will not be generated. However, when the asset store fetches asset B, then
asset C will be generated in order to generate asset B. Asset A could
potentially have data that conflicts with the data that would have been taken
from the asset C that was generated.

The new load function creates new asset instances to store the asset state loaded
from on-disk and from the state file. The store tests were relying on the same
asset instance being used throughout the test. Unfortunately, the tests now
need to use a lot of global variables, making the tests more fragile.

The assetToPurge field was removed since the same information can be obtained by
iterating over the assets map. Also, the parameter passed to the purge function
was changed to a single Asset instead of an Asset slice since the function is
only ever called with a single Asset.

Fixes #545
@abhinavdahiya

Copy link
Copy Markdown
Contributor

The changes look great. Thanks! 😇
/approve

@abhinavdahiya

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2018
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, staebler

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [abhinavdahiya,staebler]

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

@openshift-merge-robot
openshift-merge-robot merged commit 4ebf8f4 into openshift:master Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Installer May Be Ignoring Mulitple Settings in install-config.yml

4 participants