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.

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

Fix create nodepool azure command #1118

Merged
merged 1 commit into from
Mar 9, 2022
Merged

Fix create nodepool azure command #1118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions api/v1alpha1/nodepool_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,8 +428,11 @@ type AgentNodePoolPlatform struct {
}

type AzureNodePoolPlatform struct {
VMSize string `json:"vmsize"`
ImageID string `json:"imageID"`
VMSize string `json:"vmsize"`
// ImageID is the id of the image to boot from. If unset, the default image at the location below will be used:
// subscription/$subscriptionID/resourceGroups/$resourceGroupName/providers/Microsoft.Compute/images/rhcos.x86_64.vhd
// +optional
ImageID string `json:"imageID,omitempty"`
// +kubebuilder:default:=120
// +kubebuilder:validation:Minimum=16
// +optional
Expand Down
4 changes: 3 additions & 1 deletion cmd/install/assets/hypershift-operator/hypershift.openshift.io_nodepools.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,11 +423,13 @@ spec:
minimum: 16
type: integer
imageID:
description: 'ImageID is the id of the image to boot from.
If unset, the default image at the location below will be
Copy link
Member

Choose a reason for hiding this comment

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

Having a hardcoded default seems like a long term bad idea. I think we should error out if we can't find the right image instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no public azure images, only public azure image blobs, which need to be copied and referenced from an image in order to be usable. What we construct here is that reference. If we wouldn't do this, we have to store the location somewhere even if we have no nodepools, which would only leave the cluster and that doesn't really seem appropriate and might cause issues if we need more images in the future, for example because of a different arch or because we have a windows node.

Copy link
Member

Choose a reason for hiding this comment

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

Having a hardcoded default seems like a long term bad idea. I think we should error out if we can't find the right image instead.

We default the backend for a zero friction happy path while still enabling other required scenarios by exposing input at the API.
In any case if the targeted image can't be found the capi Machine will fail and the error will be bubbled up as a NodePool.status.condition (eventually, PR in flight for capi upstream for bubbling up individual Machine errors to MahcineDeployments). Alternatively we could explicitly check the image existence from our controller and fail early but keeping our controller as slim and declarative as possible while delegating logic/implementation into capi is intended so letting the Machine fail and signalling the error back seems reasonable to me.

used: subscription/$subscriptionID/resourceGroups/$resourceGroupName/providers/Microsoft.Compute/images/rhcos.x86_64.vhd'
type: string
vmsize:
type: string
required:
- imageID
- vmsize
type: object
ibmcloud:
Expand Down
20 changes: 17 additions & 3 deletions cmd/nodepool/azure/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ func NewCreateCommand(coreOpts *core.CreateNodePoolOptions) *cobra.Command {
Short: "Creates an Azure nodepool",
SilenceUsage: true,
}
o := &opts{
instanceType: "Standard_D4s_v4",
diskSize: 120,
}
cmd.Flags().StringVar(&o.instanceType, "instance-type", o.instanceType, "The instance type to use for the nodepool")
cmd.Flags().Int32Var(&o.diskSize, "root-disk-size", o.diskSize, "The size of the root disk for machines in the NodePool (minimum 16)")

cmd.Run = func(cmd *cobra.Command, args []string) {
ctx, cancel := context.WithCancel(context.Background())
Expand All @@ -29,7 +35,7 @@ func NewCreateCommand(coreOpts *core.CreateNodePoolOptions) *cobra.Command {
cancel()
}()

if err := coreOpts.CreateNodePool(ctx, opts{}); err != nil {
if err := coreOpts.CreateNodePool(ctx, o); err != nil {
log.Log.Error(err, "Failed to create nodepool")
os.Exit(1)
}
Expand All @@ -38,9 +44,17 @@ func NewCreateCommand(coreOpts *core.CreateNodePoolOptions) *cobra.Command {
return cmd
}

type opts struct{}
type opts struct {
instanceType string
diskSize int32
}

func (opts) UpdateNodePool(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster, client crclient.Client) error {
func (o *opts) UpdateNodePool(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster, client crclient.Client) error {
nodePool.Spec.Platform.Type = hyperv1.AzurePlatform
nodePool.Spec.Platform.Azure = &hyperv1.AzureNodePoolPlatform{
VMSize: o.instanceType,
DiskSizeGB: o.diskSize,
}
return nil
}

Expand Down
3 changes: 3 additions & 0 deletions docs/content/reference/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1521,6 +1521,9 @@ string
</em>
</td>
<td>
<em>(Optional)</em>
<p>ImageID is the id of the image to boot from. If unset, the default image at the location below will be used:
subscription/$subscriptionID/resourceGroups/$resourceGroupName/providers/Microsoft.Compute/images/rhcos.x86_64.vhd</p>
</td>
</tr>
<tr>
Expand Down
4 changes: 3 additions & 1 deletion hack/app-sre/saas_template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22169,11 +22169,13 @@ objects:
minimum: 16
type: integer
imageID:
description: 'ImageID is the id of the image to boot from.
If unset, the default image at the location below will
be used: subscription/$subscriptionID/resourceGroups/$resourceGroupName/providers/Microsoft.Compute/images/rhcos.x86_64.vhd'
type: string
vmsize:
type: string
required:
- imageID
- vmsize
type: object
ibmcloud:
Expand Down
12 changes: 10 additions & 2 deletions hypershift-operator/controllers/nodepool/azure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@ import (
"encoding/base64"
"fmt"

hyperv1 "github.com/openshift/hypershift/api/v1alpha1"
"golang.org/x/crypto/ssh"
utilpointer "k8s.io/utils/pointer"
capiazure "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"

hyperv1 "github.com/openshift/hypershift/api/v1alpha1"
)

func azureMachineTemplateSpec(hcluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, existing capiazure.AzureMachineTemplateSpec) (*capiazure.AzureMachineTemplateSpec, error) {
Expand All @@ -25,7 +26,7 @@ func azureMachineTemplateSpec(hcluster *hyperv1.HostedCluster, nodePool *hyperv1
}
return &capiazure.AzureMachineTemplateSpec{Template: capiazure.AzureMachineTemplateResource{Spec: capiazure.AzureMachineSpec{
VMSize: nodePool.Spec.Platform.Azure.VMSize,
Image: &capiazure.Image{ID: &nodePool.Spec.Platform.Azure.ImageID},
Image: &capiazure.Image{ID: utilpointer.String(bootImage(hcluster, nodePool))},
OSDisk: capiazure.OSDisk{
DiskSizeGB: utilpointer.Int32Ptr(nodePool.Spec.Platform.Azure.DiskSizeGB),
},
Expand All @@ -49,3 +50,10 @@ func generateSSHPubkey() (string, error) {

return base64.StdEncoding.EncodeToString(ssh.MarshalAuthorizedKey(publicRsaKey)), nil
}

func bootImage(hcluster *hyperv1.HostedCluster, nodepool *hyperv1.NodePool) string {
if nodepool.Spec.Platform.Azure.ImageID != "" {
return nodepool.Spec.Platform.Azure.ImageID
}
return fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/images/rhcos.x86_64.vhd", hcluster.Spec.Platform.Azure.SubscriptionID, hcluster.Spec.Platform.Azure.ResourceGroupName)
}
42 changes: 42 additions & 0 deletions hypershift-operator/controllers/nodepool/azure_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package nodepool

import (
"testing"

hyperv1 "github.com/openshift/hypershift/api/v1alpha1"
)

func TestBootImage(t *testing.T) {
testCases := []struct {
name string
hcluster *hyperv1.HostedCluster
nodepool *hyperv1.NodePool
expected string
}{
{
name: "Nodepool has image set, it is being used",
nodepool: &hyperv1.NodePool{Spec: hyperv1.NodePoolSpec{Platform: hyperv1.NodePoolPlatform{Azure: &hyperv1.AzureNodePoolPlatform{
ImageID: "nodepool-image",
}}}},
expected: "nodepool-image",
},
{
name: "Default bootimage is used",
hcluster: &hyperv1.HostedCluster{Spec: hyperv1.HostedClusterSpec{Platform: hyperv1.PlatformSpec{Azure: &hyperv1.AzurePlatformSpec{
SubscriptionID: "123-123",
ResourceGroupName: "rg-name",
}}}},
nodepool: &hyperv1.NodePool{Spec: hyperv1.NodePoolSpec{Platform: hyperv1.NodePoolPlatform{Azure: &hyperv1.AzureNodePoolPlatform{}}}},
expected: "/subscriptions/123-123/resourceGroups/rg-name/providers/Microsoft.Compute/images/rhcos.x86_64.vhd",
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
result := bootImage(tc.hcluster, tc.nodepool)
if result != tc.expected {
t.Errorf("expected %s, got %s", tc.expected, result)
}
})
}
}