Skip to content

Support 'none' platform #266

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.

Sign up for GitHub

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

Jump to bottom
Merged
openshift-merge-robot merged 3 commits into from
Jan 8, 2019
Merged

Support 'none' platform #266

openshift-merge-robot merged 3 commits into from
Jan 8, 2019

Conversation

@vrutkovs
Copy link
Contributor

@vrutkovs vrutkovs commented Jan 3, 2019 •
edited
Loading

This adds a support for 'none' platform for BYOR case, see openshift/installer#982

TODO:

  • Update vendored installer once the PR is merged

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 3, 2019
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 4, 2019
@vrutkovs vrutkovs changed the title WIP: Support 'none' platform Support 'none' platform Jan 4, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 4, 2019
@vrutkovs vrutkovs mentioned this pull request Jan 4, 2019
Closed
@vrutkovs
Copy link
Contributor Author

vrutkovs commented Jan 4, 2019

Verified that it no longer crashes with unsupported platform error on BYOR install

/retest

ashcrow
ashcrow reviewed Jan 4, 2019
View reviewed changes
Copy link
Member

@ashcrow ashcrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions ... one in the comments of the review. The other is if https://github.com/openshift/machine-config-operator/tree/ed21a002560e519d282da6f28eb343a7650f3abb/templates/master/00-master/none/libvirt/ is actually needed as a directory or if it snuck in while debugging.

pkg/controller/template/test_data/templates/master/01-master-kubelet/none/units/kubelet.service Outdated
Copy link
Member

@ashcrow ashcrow Jan 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For aws this line denotes aws. For openstack the --cloud-provider line is not listed. Should this be removed or is there a reason it needs to be set empty?

Copy link
Member

@ashcrow ashcrow Jan 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(note this is the case with the other unit file in this PR as well)

Copy link
Member

@ashcrow ashcrow Jan 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vrutkovs spoke with me in chat and will verify the difference here. If needed he'll do a follow on PR.

Copy link
Contributor Author

@vrutkovs vrutkovs Jan 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bparees @sjenning might know the difference, I simply copied this from libvirt. CI would run this on GCP, so we don't want it to accidentally guess that

Copy link
Contributor Author

@vrutkovs vrutkovs Jan 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also @rajatchopra might know more about that

Copy link

@rajatchopra rajatchopra Jan 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its empty by default, so removing it or giving it empty string is the same. Eventually we will move this argument to value 'external'. But this is okay for now.

@cgwalters
Copy link
Member

cgwalters commented Jan 4, 2019

I don't know this code very well, but it seems to me if we're going to add any more platforms we'll need to invest in de-duplicating this stuff; factoring out a "base" platform and capture only things that differ as e.g. systemd drop-ins or separately rendered/templated files.

@ashcrow
Copy link
Member

ashcrow commented Jan 7, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 7, 2019
@ashcrow
Copy link
Member

ashcrow commented Jan 7, 2019

/test e2e-aws

3 similar comments
@vrutkovs
Copy link
Contributor Author

vrutkovs commented Jan 7, 2019

/test e2e-aws

@ashcrow
Copy link
Member

ashcrow commented Jan 7, 2019

/test e2e-aws

@vrutkovs
Copy link
Contributor Author

vrutkovs commented Jan 7, 2019

/test e2e-aws

@crawford
Copy link
Contributor

crawford commented Jan 7, 2019

This looks good to me. Would you mind breaking the functional addition and the generation of test data into two separate commits? We've done that in the past to make it more clear what's actually changing.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 7, 2019
@vrutkovs
Copy link
Contributor Author

vrutkovs commented Jan 7, 2019

Would you mind breaking the functional addition and the generation of test data into two separate commits?

Sure, done

@crawford
Copy link
Contributor

crawford commented Jan 7, 2019

/approve

@abhinavdahiya can you take a quick peek?

@vrutkovs
Copy link
Contributor Author

vrutkovs commented Jan 7, 2019

/test e2e-aws

@ashcrow
Copy link
Member

ashcrow commented Jan 7, 2019

Flake

/retest

@ashcrow
Copy link
Member

ashcrow commented Jan 7, 2019

/test e2e-aws

1 similar comment
@vrutkovs
Copy link
Contributor Author

vrutkovs commented Jan 7, 2019

/test e2e-aws

wking
wking reviewed Jan 7, 2019
View reviewed changes
pkg/operator/render.go
return "openstack"
case ic.Libvirt != nil:
return "libvirt"
case ic.None != nil:
Copy link
Member

@wking wking Jan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not replace platformFromInstallConfig(config) with config.Platform.Name()? Or use config.Platform.Name() inside platformFromInstallConfig and raise errors on empty-string results?

Copy link
Contributor Author

@vrutkovs vrutkovs Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather handle this in a different PR, I'm not sure if other components are relying on the function - and unblocking BYOR ASAP is the priority

@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Jan 7, 2019

/approve

@vrutkovs i can lgtm if you think #266 (comment) should be handled in separate PR.

@vrutkovs
Copy link
Contributor Author

vrutkovs commented Jan 8, 2019

/test e2e-aws

1 similar comment
@vrutkovs
Copy link
Contributor Author

vrutkovs commented Jan 8, 2019

/test e2e-aws

@ashcrow
Copy link
Member

ashcrow commented Jan 8, 2019

/lgtm

Follow up PRs will address code enhancements.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 8, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, ashcrow, crawford, vrutkovs

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,ashcrow,crawford]

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 5256cd0 into openshift:master Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashcrow ashcrow ashcrow left review comments

@wking wking wking left review comments

@cgwalters cgwalters Awaiting requested review from cgwalters

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

+1 more reviewer

@rajatchopra rajatchopra rajatchopra left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ashcrow ashcrow

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@vrutkovs @cgwalters @ashcrow @crawford @abhinavdahiya @openshift-ci-robot @wking @rajatchopra @openshift-merge-robot