Skip to content

Conversation

@Prashanth684
Copy link
Contributor

With this change the bootindex is changed dynamically rather than being statically fed in during VM creation.
One of the problematic areas was s390x where the network device needs to have bootindex 1 but on subsequent boots
it needs to boot from disk. This helps in that situation of not setting a bootindex for the primary disk initially
but setting it later to avoid the bootindex conflict.

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Prashanth684
To complete the pull request process, please assign ashcrow
You can assign the PR to them by writing /assign @ashcrow in a comment when ready.

The full list of commands accepted by this bot can be found 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

@Prashanth684
Copy link
Contributor Author

Taking a stab at @cgwalters suggestion from #1336

@Prashanth684
Copy link
Contributor Author

@cgwalters or somebody - could you help me with updating the vendor for github.com/digitalocean/go-qemu/ ? I did try with an older version of the assembler and tested with that and it works fine. But on the latest assembler, when i try a go mod tidy and go mod vendor i get build errors. I am also not super comfortable with go modules in general and I am afraid I will mess something up.

@arithx
Copy link
Contributor

arithx commented Apr 15, 2020

@cgwalters or somebody - could you help me with updating the vendor for github.com/digitalocean/go-qemu/ ? I did try with an older version of the assembler and tested with that and it works fine. But on the latest assembler, when i try a go mod tidy and go mod vendor i get build errors. I am also not super comfortable with go modules in general and I am afraid I will mess something up.

What error were you specifically seeing?

@cgwalters
Copy link
Member

I'm not seeing any changes to metal.go - I'd expect to see that because basically we need to do:

  • PXE boot
  • [guest] systemd unit like rebootUnit in metal.go except writes to a virtio-serial channel (hm, needs to block and wait for response, so we need bidirectional channels)
  • [host] read from virtio-serial channel and uses QMP to adjust bootorder
  • [guest] continue reboot

right?

@Prashanth684
Copy link
Contributor Author

Prashanth684 commented Apr 15, 2020

@cgwalters or somebody - could you help me with updating the vendor for github.com/digitalocean/go-qemu/ ? I did try with an older version of the assembler and tested with that and it works fine. But on the latest assembler, when i try a go mod tidy and go mod vendor i get build errors. I am also not super comfortable with go modules in general and I am afraid I will mess something up.

What error were you specifically seeing?

When I do a go mod tidy and a go mod vendor in the mantle directory and try a build - i see this:

09:16:19 psundara@psundara mantle$./build
Building kola
# github.com/coreos/mantle/platform/api/openstack
platform/api/openstack/api.go:363:13: undefined: groups.IDFromName
09:16:36 psundara@psundara mantle$

@Prashanth684
Copy link
Contributor Author

I'm not seeing any changes to metal.go - I'd expect to see that because basically we need to do:

  • PXE boot
  • [guest] systemd unit like rebootUnit in metal.go except writes to a virtio-serial channel (hm, needs to block and wait for response, so we need bidirectional channels)
  • [host] read from virtio-serial channel and uses QMP to adjust bootorder
  • [guest] continue reboot

right?

Hmm..Ok sorry I think I misunderstood your initial comment then - i did this change just from the perspective of setting the bootindex dynamically so the s390x testiso would work. Would it be possible that we can make the change incrementally and I will let you/someone else pick up the rest of the changes?

@cgwalters
Copy link
Member

Would it be possible that we can make the change incrementally

Yep #1336 (comment)

@Prashanth684
Copy link
Contributor Author

Would it be possible that we can make the change incrementally

Yep #1336 (comment)

Opened #1350. Couldn't reopen the old one as i had already rebased it. sorry for the back and forth.

@arithx
Copy link
Contributor

arithx commented Apr 15, 2020

@cgwalters or somebody - could you help me with updating the vendor for github.com/digitalocean/go-qemu/ ? I did try with an older version of the assembler and tested with that and it works fine. But on the latest assembler, when i try a go mod tidy and go mod vendor i get build errors. I am also not super comfortable with go modules in general and I am afraid I will mess something up.

What error were you specifically seeing?

When I do a go mod tidy and a go mod vendor in the mantle directory and try a build - i see this:

09:16:19 psundara@psundara mantle$./build
Building kola
# github.com/coreos/mantle/platform/api/openstack
platform/api/openstack/api.go:363:13: undefined: groups.IDFromName
09:16:36 psundara@psundara mantle$

The groups.IDFromName function that isn't being found is a call being made to the openstack API. It seems like they've dropped it in whichever version you're trying to bump to. You'd probably just need to find the equivalent function or re-work the existing getSecurityGroup function to use a different function resulting in the same functionality.

@Prashanth684
Copy link
Contributor Author

@cgwalters or somebody - could you help me with updating the vendor for github.com/digitalocean/go-qemu/ ? I did try with an older version of the assembler and tested with that and it works fine. But on the latest assembler, when i try a go mod tidy and go mod vendor i get build errors. I am also not super comfortable with go modules in general and I am afraid I will mess something up.

What error were you specifically seeing?

When I do a go mod tidy and a go mod vendor in the mantle directory and try a build - i see this:

09:16:19 psundara@psundara mantle$./build
Building kola
# github.com/coreos/mantle/platform/api/openstack
platform/api/openstack/api.go:363:13: undefined: groups.IDFromName
09:16:36 psundara@psundara mantle$

The groups.IDFromName function that isn't being found is a call being made to the openstack API. It seems like they've dropped it in whichever version you're trying to bump to. You'd probably just need to find the equivalent function or re-work the existing getSecurityGroup function to use a different function resulting in the same functionality.

Ok..i can do that..but is there a way through go mod to just add a single dependent package? not sure there is.

@ashcrow ashcrow requested review from darkmuggle and removed request for ashcrow April 15, 2020 17:20
@arithx
Copy link
Contributor

arithx commented Apr 15, 2020

Ok..i can do that..but is there a way through go mod to just add a single dependent package? not sure there is.

If you mean bumping the version only for a specific package of the larger library (thus leaving the other packages at a different version) then to my knowledge there isn't a way. If you mean adding only a specific package from a library then there should be, iirc it only adds the packages being used from a library.

@jlebon
Copy link
Member

jlebon commented Apr 15, 2020

Using QMP to change the bootindex is nice!

I think this also allows us to stop special-casing s390x as done in #1350, right? (See also #1350 (comment)).

With this change the bootindex is changed dynamically rather than being statically fed in during VM creation.
One of the problematic areas was s390x where the network device needs to have bootindex 1 but on subsequent boots
it needs to boot from disk. This helps in that situation of not setting a bootindex for the primary disk initially
but setting it later to avoid the bootindex conflict.
@Prashanth684
Copy link
Contributor Author

Ok..i can do that..but is there a way through go mod to just add a single dependent package? not sure there is.

If you mean bumping the version only for a specific package of the larger library (thus leaving the other packages at a different version) then to my knowledge there isn't a way. If you mean adding only a specific package from a library then there should be, iirc it only adds the packages being used from a library.

Using QMP to change the bootindex is nice!

I think this also allows us to stop special-casing s390x as done in #1350, right? (See also #1350 (comment)).

Correct. But this is just a part of the bigger changes that @cgwalters is suggesting above

@Prashanth684
Copy link
Contributor Author

Ok..i can do that..but is there a way through go mod to just add a single dependent package? not sure there is.

If you mean bumping the version only for a specific package of the larger library (thus leaving the other packages at a different version) then to my knowledge there isn't a way. If you mean adding only a specific package from a library then there should be, iirc it only adds the packages being used from a library.

Thanks! was able to pull just that module in and got it to work.

@cgwalters
Copy link
Member

[guest] systemd unit like rebootUnit in metal.go except writes to a virtio-serial channel (hm, needs to block and wait for response, so we need bidirectional channels)

Or, more closely matching Foreman/Ironic etc. - run a webserver on the host and have it accept a POST which switches the bootorder.

@Prashanth684
Copy link
Contributor Author

I'm not seeing any changes to metal.go - I'd expect to see that because basically we need to do:

  • PXE boot
  • [guest] systemd unit like rebootUnit in metal.go except writes to a virtio-serial channel (hm, needs to block and wait for response, so we need bidirectional channels)
  • [host] read from virtio-serial channel and uses QMP to adjust bootorder
  • [guest] continue reboot

right?

@cgwalters i'm afraid i need some clarity on this. I think my understanding is not sufficient. I thought setting the bootindex in the builder.Exec would take care of all cases including metal and it would also be valid on reboots. What is it that we are trying to achieve here?

@cgwalters
Copy link
Member

No worries; I can take this at some point hopefully soon. Thanks for starting this!

What is it that we are trying to achieve here?

To dynamically switch the boot order after an install is complete, before the reboot.

@openshift-ci-robot
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants