Skip to content
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

Progress on Proposal 0016, Default Workflows #179

Closed
wants to merge 5 commits into from

Conversation

Belchy06
Copy link

@Belchy06 Belchy06 commented Jul 22, 2021

Description

This PR adds the ability for Boots to push hardware from a currently unknown mac address.

Currently the hardware template is a hardcoded, minimalistic template from the hello-world tutorial. Proper hardware discovery, or input from a default hardware file will be needed.

Why is this needed

This feature is required for the project I am working on but I am sure there are many other use cases I'm unaware of. Also, #178

This PR requires additional functionality from the TInk-CLI which can be seen: tinkerbell/tink#516

How Has This Been Tested?

This has been tested in the VM's based on the Local Setup with Vagrant tutorial.

If the env var ENABLE_DEFAULT_WORKFLOWS is not set, Boots will follow it's original code path meaning that mass destruction can be prevented

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

…ady exist

Signed-off-by: William Belcher <william.belcher@hotmail.com>
Signed-off-by: Belchy06 <william.belcher@hotmail.com>
…ardware from unknown mac

Signed-off-by: William Belcher <william.belcher@hotmail.com>
Signed-off-by: Belchy06 <william.belcher@hotmail.com>
Signed-off-by: Belchy06 <william.belcher@hotmail.com>
Signed-off-by: Belchy06 <william.belcher@hotmail.com>
@Belchy06 Belchy06 marked this pull request as ready for review July 29, 2021 05:52
@jacobweinstock jacobweinstock linked an issue Aug 24, 2021 that may be closed by this pull request
Copy link
Contributor

@tstromberg tstromberg left a comment

Choose a reason for hiding this comment

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

Some minor changes, will defer to @jacobweinstock for a more detailed review.

@@ -3,7 +3,7 @@
set -eu

# Deps on ubuntu
# apt-get -y --no-install-recommends install build-essential gcc-aarch64-linux-gnu git liblzma-dev
sudo apt-get -y --no-install-recommends install build-essential gcc-aarch64-linux-gnu git liblzma-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes to build.sh seem unrelated - can you move them to another PR?

if err != nil {
return nil, err
}
return v.(packet.Discovery), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

This should catch a type assertion error instead of panic'ing:

d, ok := v.(packet.Discovery)
if !ok {
  return nil, fmt.Errorf("unable to cast %+v to packet.Discovery")
}

return d, nil

@@ -90,6 +112,25 @@ func CreateFromDHCP(mac net.HardwareAddr, giaddr net.IP, circuitID string) (Job,
return j, nil
}

// CreateWorkflowFromDHCP creates a workflow and
func CreateHWFromDHCP(mac net.HardwareAddr, giaddr net.IP, circuitID string) (Job, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be called CreateWorkflowFromDHCP?


b, err := json.Marshal(&req)
if err != nil {
return nil, errors.Wrap(err, "unmarshalling api discovery")
Copy link
Contributor

Choose a reason for hiding this comment

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

the error should be marshaling. errors.Wrap(err, "marshal") is honestly good enough.

@jacobweinstock
Copy link
Member

jacobweinstock commented Aug 27, 2021

@Belchy06 thanks so much for this work! I want to make sure I understand fully what you're looking to do. Do I have this correct?

  1. hardware discovery
    • a machine requesting a PXE boot without a corresponding Hardware record in Tink Server will cause Boots to push a new minimal Hardware record to Tink Server.
  2. default workflow
    • a machine requesting a PXE boot without a corresponding Workflow in Tink Server will cause Boots to create a new Workflow in Tink Server using an existing Template named "default" and the MAC address in the DHCP request.

Assuming these 2 are accurate statements, the following might need to be discussed further.

hardware discovery

  • I think we might need to implement some kind of IP Pool feature from which an IP can be used in the creation of a Tink Server Hardware record.

default workflow

  • This could potentially be functionality that lives entirely in Tink Server

@Belchy06
Copy link
Author

@jacobweinstock Your two points were exactly my train of thought.

I implemented the IP assignment functionality on a separate branch here. In this case the user just specifies the starting IP and a lease range and Boots takes care of the rest.

This solution feels quite 'hacky' and feel like quality code, hence why it's not in this PR.

Keen to discuss your ideas further!

@djpbessems
Copy link

Very interested in this; this could allow my Tinkerbell Appliance to automatically build an inventory from which endusers will be able to select and assign images. Would be super :)

@tstromberg
Copy link
Contributor

@Belchy06 - would you have time to resolve the PR conflicts & minor issues I raised?

@tstromberg
Copy link
Contributor

@Belchy06 - Do you need any additional help or advice to push this PR forward? We'd be happy to discuss it here, on the Tinkerbell Slack, or in the community meeting next week: https://tinkerbell.org/community/

@Belchy06
Copy link
Author

Belchy06 commented Oct 7, 2021

@tstromberg Sorry Thomas.

I'm currently in the middle of my university semester and juggling that with work as led to this falling to the back burner.

I've got around 6-7 weeks before I'll be able to commit some of my time again to working on this, and I am happy to fix the issues and get this merged when I've got the time again.

If that's too long just let me know.

@jacobweinstock
Copy link
Member

Hey @Belchy06, thank you so much for working on this! No worries at all for the delay. I'm going to close this for now, but please do re-open this when you are back with time. Best of luck and well wishes on your studies!

@jmpolom
Copy link

jmpolom commented Oct 24, 2021

This is a super interesting concept that I’d like to suggest we examine generalizing further. It would be extremely nice if boots could support functioning without the tink workflow engine for cases where it will be used to provision a relatively small set of systems and an ultra lightweight deployment is desirable. I will get around to opening a separate issue to detail what I think would be a useful feature set here, but for now I’d like to convey that this is stellar initial work.

@nicklasfrahm
Copy link

I will throw in a design doc once I am done with some work on the integration with Kubernetes.

@jmpolom
Copy link

jmpolom commented Feb 15, 2022

@nicklasfrahm is there a way to provide input and feedback on this design document? I am extremely interested in participating in the design of this feature.

@nicklasfrahm
Copy link

Absolutely, I will poke you once I start the PR. We could hop on call or something.

@jmpolom
Copy link

jmpolom commented Feb 15, 2022

@nicklasfrahm yes please reach out. When do you envision starting this process? I could accommodate a call on pretty short notice but would prefer to plan something out a bit if possible.

@nicklasfrahm
Copy link

Are you on the CNCF slack? Throw me a DM there and we'll make it work!

@jmpolom
Copy link

jmpolom commented Feb 15, 2022

I am not. I believe that is invite only? Can you send me an invite?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal 0016, Default Workflows
6 participants