Skip to content

[FCOS] support libvirt install#2678

Closed
vrutkovs wants to merge 34 commits intoopenshift:fcosfrom
vrutkovs:fcos-libvirt
Closed

[FCOS] support libvirt install#2678
vrutkovs wants to merge 34 commits intoopenshift:fcosfrom
vrutkovs:fcos-libvirt

Conversation

@vrutkovs
Copy link
Copy Markdown
Contributor

@vrutkovs vrutkovs commented Nov 15, 2019

  • Update links to FCOS qcow
  • Extract xz-compressed images in libvirt

Requires openshift/release#5933 to add libvirt test.

Fixes okd-project/okd#15

Vadim Rutkovsky and others added 30 commits October 24, 2019 20:49
[fcos] Add vrutkovs and LorbusChris to OWNERS
This PR updated the current building system and ci images from
golang 1.10 to 1.12.
Rather than doing a `systemctl restart crio` after `kubelet.service`
has started, separate out the bit that changes the pause image
into a small separate service.
The kubelet template in the MCO should be considered canonical.
We want the crio.service dependencies in particular.
If one of these commands happens to exit with an error,
we don't want to blindly stumble on.

Furthermore, `-u` helps guard against typos.

http://redsymbol.net/articles/unofficial-bash-strict-mode/
[fcos] Support Ignition3 and FCOS
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 15, 2019
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2019
Comment thread pkg/tfvars/internal/cache/cache.go Outdated
Comment thread pkg/tfvars/internal/cache/cache.go Outdated
@vrutkovs
Copy link
Copy Markdown
Contributor Author

/hold

until openshift/release#5933 lands so that it could be properly tested

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2019
Comment thread data/data/rhcos.json
"size": 700315704,
"uncompressed-sha256": "b4cd810376dca356f987e2e7f8609f84d63720d5ad19d3101f84e82d1918c85e",
"uncompressed-size": 1889337344
"path": "fedora-coreos-30.20191014.0-qemu.x86_64.qcow2.xz",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is going to break baremetal, we rely on the QEMU image for the bootstrap node - it seems odd to have mixed RHCOS/FCOS content here, shouldn't it be one or the other for everything?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also the next time someone runs the hack script to update the RHCOS build this will all get overwritten.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Baremetal IPI is currently not supported on OKD-on-FCOS - Ironic bits need to be replaced with UBI8 + RDO, hack script updated etc. I'll make another PR once it gets there

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right I get that it won't work with OKD-on-FCOS, but this is breaking baremetal IPI entirely. Is all of openshift/install master moving to FCOS and too bad for any platforms that don't work with it yet?

Copy link
Copy Markdown
Member

@stbenjam stbenjam Nov 15, 2019

Choose a reason for hiding this comment

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

My concern here is baremetal IPI has been broken for weeks, and we're very close to recovering from it and having working CI again. Now, with this change, our Metal3 CI can't be run against anything in master, so we really have no idea if some future commits break us until it ends up in a release branch :-(

Copy link
Copy Markdown
Member

@stbenjam stbenjam Nov 15, 2019

Choose a reason for hiding this comment

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

Sorry for the noise, it was just pointed out to me this isn't going to the master branch. I'm keen to get baremetal IPI working with FCOS but doesn't need to hold up this patch. Disregard me 🤦‍♂️

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it seems odd to have mixed RHCOS/FCOS content here, shouldn't it be one or the other for everything?

@vrutkovs I think this very important question from @stbenjam wasn't answered. If the answer is that we're just moving everything to FCOS, I think we should rename the file and (more importantly) commit log should reflect the reason for the change.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This PR is only for the fcos branch, it won't affect main development on master.

Comment thread pkg/rhcos/builds.go
SHA256 string `json:"sha256"`
Path string `json:"path"`
SHA256 string `json:"sha256"`
UncompressedSHA256 string `json:"uncompressed-sha256"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some info in the commit log details on why this change is needed, would be good.

Comment thread data/data/rhcos.json
"size": 700315704,
"uncompressed-sha256": "b4cd810376dca356f987e2e7f8609f84d63720d5ad19d3101f84e82d1918c85e",
"uncompressed-size": 1889337344
"path": "fedora-coreos-30.20191014.0-qemu.x86_64.qcow2.xz",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it seems odd to have mixed RHCOS/FCOS content here, shouldn't it be one or the other for everything?

@vrutkovs I think this very important question from @stbenjam wasn't answered. If the answer is that we're just moving everything to FCOS, I think we should rename the file and (more importantly) commit log should reflect the reason for the change.

@zeenix
Copy link
Copy Markdown
Contributor

zeenix commented Nov 19, 2019

/approve cancel

@hardys hardys mentioned this pull request Nov 19, 2019
4 tasks
@vrutkovs
Copy link
Copy Markdown
Contributor Author

/retest

@vrutkovs
Copy link
Copy Markdown
Contributor Author

vrutkovs commented Dec 2, 2019

@vrutkovs I think this very important question from @stbenjam wasn't answered. If the answer is that we're just moving everything to FCOS, I think we should rename the file and (more importantly) commit log should reflect the reason for the change.

Both changes would be addressed in followup PRs. Baremetal supports depends on openstack images rebuilt from RDO (instead of OSP). rhcos.json needs a consensus from architects on the chosen name. At this point we keep the same name to avoid extra changes during regular rebases from master

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

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

Test name Commit Details Rerun command
ci/prow/e2e-vsphere b9870bb link /test e2e-vsphere
ci/prow/e2e-aws b9870bb link /test e2e-aws
ci/prow/tf-lint b9870bb link /test tf-lint
ci/prow/shellcheck b9870bb link /test shellcheck
ci/prow/yaml-lint b9870bb link /test yaml-lint

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.

Details

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.

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@vrutkovs: PR needs rebase.

Details

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 openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 31, 2020
@vrutkovs
Copy link
Copy Markdown
Contributor Author

vrutkovs commented Feb 4, 2020

No longer required - all fixes have been merged in the branch

@vrutkovs vrutkovs closed this Feb 4, 2020
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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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.

9 participants