-
Notifications
You must be signed in to change notification settings - Fork 27
add new tag pit.harvester.daily for recurring harvester runs
#463
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
add new tag pit.harvester.daily for recurring harvester runs
#463
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, it looks good. I just have a few comments
|
|
||
| p.cattleConfig = config.LoadConfigFromFile(os.Getenv(config.ConfigEnvironmentKey)) | ||
|
|
||
| p.cattleConfig, err = defaults.SetK8sDefault(client, defaults.RKE2, p.cattleConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the k8s version configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, its still pulled from the config file. But if it shows up as empty, SetK8sDefault will use the latest (which is what I want in this case, but the config value is still used as 1st priority if it is there)
| nodeRolesDedicated := []provisioninginput.MachinePools{provisioninginput.EtcdMachinePool, provisioninginput.ControlPlaneMachinePool, provisioninginput.WorkerMachinePool} | ||
| nodeRolesDedicated[0].MachinePoolConfig.Quantity = 1 | ||
| nodeRolesDedicated[1].MachinePoolConfig.Quantity = 2 | ||
| nodeRolesDedicated[2].MachinePoolConfig.Quantity = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this means 1 etcd, 2 control plane and 2 worker? why not the more standard 1 etcd, 1 control plane and 2 worker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for cloud provider, the cpi controller runs on the control plane. having 2 cp nodes is the 'production minimum' recommendation (we usually do 3etcd, 2cp, 3 workers but I'm resource limited in harvester lab)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential future enhancement to allow this to be set via config as well, but I'm fine with the hardcoded values right now due to our lab limitations.
|
@slickwarren Also just alerting that a PR check is failing due to: |
e3c0c72 to
3cc7323
Compare
floatingman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few bash suggestions
| @@ -0,0 +1,45 @@ | |||
| #!/bin/bash | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a set -eu so this script quits if there is an error, otherwise it will continue to run, which could cause problems with all of the creating and deleting going on.
| # harvester has a heavy dependency on v28 and below of docker for now. | ||
| sudo apt install -y docker-ce=5:28.5.2-1~ubuntu.24.04~noble docker-ce-cli=5:28.5.2-1~ubuntu.24.04~noble containerd.io | ||
|
|
||
| sudo usermod -aG docker ubuntu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change ubuntu to $USER in case the user running this script changes. For example, if we ever change to using SUSE Linux vs. Ubuntu
| # can run make before-hand, but I don't think its necessary if I just want images. . | ||
| # make | ||
| make build-iso | ||
| sleep 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this sleep necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sometimes, the file write at the last stage isn't quite complete for some reason after make build-iso which caused a timing issue on the next step.
|
|
||
| rm -rf /usr/local/go && tar -C /usr/local -xzf go1.25.5.linux-amd64.tar.gz | ||
|
|
||
| echo "export PATH=$PATH:/usr/local/go/bin" >> .bashrc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this to single quotes. Changes the way the $PATH variable is expanded, preventing issues that are hard to diagnose.
| listen [::]:32309; | ||
| server_name _; | ||
|
|
||
| root /home/ubuntu/harvester/dist/artifacts; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to add a comment here about changing the path if we ever change the linux distribution this is run on.
| rm -rf old/ | ||
| rm -rf latest/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want to check if these directories exist first before deleting them.
Something like this is sufficient
if [ -d "$DIRECTORY" ]; then
echo "$DIRECTORY does exist."
fi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is actually why I don't set -eu 😅 good suggestion
| @@ -0,0 +1,36 @@ | |||
| #!/bin/bash | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a set -eu here too.
| logrus.Infof("Provisioning cluster") | ||
| cluster, err := provisioning.CreateProvisioningCluster(tt.client, provider, credentialSpec, clusterConfig, machineConfigSpec, nil) | ||
| require.NoError(p.T(), err) | ||
| logrus.Infof("Provisioning cluster") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could you use the suite approche for the logs on the tests?
suggestion: p.T().Logf("Provisioning cluster")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we usually use logrus for logs
| } | ||
|
|
||
| func (p *HarvesterProvisioningTestSuite) TestHarvesterCloudProvider() { | ||
| func (p *HarvesterProvisioningTestSuite) TestCloudProvider() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could we 'h' as suite variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted p for provisioning 😄
|
@floatingman the scripts are in a 'beta' state if you will. But they're all currently used and working on the server node. I just wanted to add them here as a fallback -- Can I make a separate ticket to address your comments for it and merge as-is? |
| nodeRolesDedicated := []provisioninginput.MachinePools{provisioninginput.EtcdMachinePool, provisioninginput.ControlPlaneMachinePool, provisioninginput.WorkerMachinePool} | ||
| nodeRolesDedicated[0].MachinePoolConfig.Quantity = 1 | ||
| nodeRolesDedicated[1].MachinePoolConfig.Quantity = 2 | ||
| nodeRolesDedicated[2].MachinePoolConfig.Quantity = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential future enhancement to allow this to be set via config as well, but I'm fine with the hardcoded values right now due to our lab limitations.
also added scripts to fall back on if the 'build server' goes down