EM: Add worker pool specific facility param #1359

surajssd · 2021-02-03T13:09:40Z

This commit adds a param to the worker pools called facility. This
allows users to choose a different facility/region than the controller
nodes. The default behaviour is to deploy controllers and workers in the
same facility.

Signed-off-by: Suraj Deshmukh suraj@kinvolk.io

invidian

Seems OK if it works. Perhaps we could have some kind of guide on when to use this feature etc.

docs/configuration-reference/platforms/packet.md

surajssd · 2021-02-04T07:49:42Z

Perhaps we could have some kind of guide on when to use this feature etc.

Yep I am on it, in a separate PR, so that we don't trigger CI without reason.

surajssd · 2021-02-04T08:08:37Z

I think we need to convert node_private_cidr to list instead of a string.

invidian · 2021-02-04T08:59:11Z

I think we need to convert node_private_cidr to list instead of a string.

Maybe we add node_private_cidrS to avoid breaking change and to make names nice?

surajssd · 2021-02-05T09:42:01Z

@invidian done.

knrt10

Small NITs. Apart from CI failing, the rest looks OK

docs/configuration-reference/platforms/packet.md

assets/terraform-modules/packet/flatcar-linux/kubernetes/variables.tf

pkg/platform/packet/packet.go

assets/terraform-modules/packet/flatcar-linux/kubernetes/bootkube.tf

surajssd · 2021-02-09T09:56:47Z

@invidian @knrt10 PTAL.

pkg/platform/packet/packet.go

docs/configuration-reference/platforms/baremetal.md

docs/configuration-reference/platforms/packet.md

pkg/platform/packet/packet.go

pkg/platform/packet/packet_test.go

pkg/platform/packet/packet.go

pkg/platform/packet/packet_test.go

invidian

My concerns are still not addressed, so this should perhaps be reviewed by others.

pkg/platform/packet/packet.go

pkg/platform/packet/packet_test.go

This commit adds a param to the worker pools called facility. This allows users to choose a different facility/region than the controller nodes. The default behaviour is to deploy controllers and workers in the same facility. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

For calico's node ip auto detection use first controller node's private IP instead of node private CIDR. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

iaguis

Last thing from my side 🙏

Talking a bit more about validation, can we do this? We won't be doing random prints in pkg/platform and we warn the proper way with logrus.

diff --git a/cli/cmd/cluster/cluster.go b/cli/cmd/cluster/cluster.go
index 954f6b32a..81f3d9b5b 100644
--- a/cli/cmd/cluster/cluster.go
+++ b/cli/cmd/cluster/cluster.go
@@ -18,6 +18,7 @@ import (
        "fmt"
        "path/filepath"

+       "github.com/hashicorp/hcl/v2"
        "github.com/mitchellh/go-homedir"
        log "github.com/sirupsen/logrus"
        "helm.sh/helm/v3/pkg/action"
@@ -58,11 +59,15 @@ func (cc clusterConfig) initialize(contextLogger *log.Entry) (*cluster, error) {
        }

        p, diags := getConfiguredPlatform(lokoConfig, true)
-       if diags.HasErrors() {
-               for _, diagnostic := range diags {
+       for _, diagnostic := range diags {
+               if diagnostic.Severity == hcl.DiagError {
                        contextLogger.Error(diagnostic.Error())
+               } else {
+                       contextLogger.Warning(diagnostic.Error())
                }
+       }

+       if diags.HasErrors() {
                return nil, fmt.Errorf("loading platform configuration")
        }

diff --git a/pkg/platform/packet/packet.go b/pkg/platform/packet/packet.go
index 8ab4134a3..f2f652136 100644
--- a/pkg/platform/packet/packet.go
+++ b/pkg/platform/packet/packet.go
@@ -112,6 +112,12 @@ const (
        Name = "packet"
 )

+type DeprecatedNodeCIDRError struct{}
+
+func (e DeprecatedNodeCIDRError) Error() string {
+       return "`node_private_cidr` is deprecated, use `node_private_cidrs` instead"
+}
+
 func (c *config) LoadConfig(configBody *hcl.Body, evalContext *hcl.EvalContext) hcl.Diagnostics {
        if configBody == nil {
                return hcl.Diagnostics{}
@@ -240,7 +246,12 @@ func createTerraformConfigFile(cfg *config, terraformPath string) error {

        nodePrivateCIDRs, err := validateNodePrivateCIDRs(cfg)
        if err != nil {
-               return fmt.Errorf("validating Node Private CIDR: %w", err)
+               switch err.(type) {
+               case DeprecatedNodeCIDRError:
+                       // we've warned about this before, ignore
+               default:
+                       return fmt.Errorf("validating Node Private CIDR: %w", err)
+               }
        }

        // Configure oidc flags and set it to KubeAPIServerExtraFlags.
@@ -306,9 +317,7 @@ func validateNodePrivateCIDRs(cfg *config) ([]string, error) {

        // If just the deprecated field is provided then produce a warning.
        if cfg.NodePrivateCIDR != "" {
-               fmt.Printf("Warning: `node_private_cidr` is deprecated, use `node_private_cidrs` instead.\n")
-
-               return []string{cfg.NodePrivateCIDR}, nil
+               return []string{cfg.NodePrivateCIDR}, DeprecatedNodeCIDRError{}
        }

        // If there are repeated CIDRs then error out.
@@ -480,10 +489,19 @@ func (c *config) checkValidConfig() hcl.Diagnostics {
        }

        if _, err := validateNodePrivateCIDRs(c); err != nil {
+               var severity hcl.DiagnosticSeverity
+
+               switch err.(type) {
+               case DeprecatedNodeCIDRError:
+                       severity = hcl.DiagWarning
+               default:
+                       severity = hcl.DiagError
+               }
+
                diagnostics = append(diagnostics, &hcl.Diagnostic{
-                       Severity: hcl.DiagError,
+                       Severity: severity,
                        Summary:  "Node Private CIDR",
-                       Detail:   fmt.Sprintf("Node Private CIDR failed with: %v", err),
+                       Detail:   fmt.Sprintf("Validating Node Private CIDR: %v", err),
                })
        }

invidian · 2021-02-26T12:36:33Z

I think validateNodePrivateCIDRs could return diagnostics directly, this way we don't need special error type, which we don't use much here. Otherwise looks good to me @iaguis.

iaguis · 2021-02-26T13:02:02Z

I think validateNodePrivateCIDRs could return diagnostics directly, this way we don't need special error type, which we don't use much here. Otherwise looks good to me @iaguis.

Right, so something like this? (I hate using hcl.Diagnostics)

I can add this as a commit on top if @surajssd agrees.

diff --git a/cli/cmd/cluster/cluster.go b/cli/cmd/cluster/cluster.go
index 954f6b32a..81f3d9b5b 100644
--- a/cli/cmd/cluster/cluster.go
+++ b/cli/cmd/cluster/cluster.go
@@ -18,6 +18,7 @@ import (
        "fmt"
        "path/filepath"

+       "github.com/hashicorp/hcl/v2"
        "github.com/mitchellh/go-homedir"
        log "github.com/sirupsen/logrus"
        "helm.sh/helm/v3/pkg/action"
@@ -58,11 +59,15 @@ func (cc clusterConfig) initialize(contextLogger *log.Entry) (*cluster, error) {
        }

        p, diags := getConfiguredPlatform(lokoConfig, true)
-       if diags.HasErrors() {
-               for _, diagnostic := range diags {
+       for _, diagnostic := range diags {
+               if diagnostic.Severity == hcl.DiagError {
                        contextLogger.Error(diagnostic.Error())
+               } else {
+                       contextLogger.Warning(diagnostic.Error())
                }
+       }

+       if diags.HasErrors() {
                return nil, fmt.Errorf("loading platform configuration")
        }

diff --git a/pkg/platform/packet/packet.go b/pkg/platform/packet/packet.go
index 8ab4134a3..f9684d7d0 100644
--- a/pkg/platform/packet/packet.go
+++ b/pkg/platform/packet/packet.go
@@ -238,9 +238,9 @@ func createTerraformConfigFile(cfg *config, terraformPath string) error {
                return fmt.Errorf("marshaling management CIDRs: %w", err)
        }

-       nodePrivateCIDRs, err := validateNodePrivateCIDRs(cfg)
-       if err != nil {
-               return fmt.Errorf("validating Node Private CIDR: %w", err)
+       nodePrivateCIDRs, diags := validateNodePrivateCIDRs(cfg)
+       if diags.HasErrors() {
+               return fmt.Errorf("validating Node Private CIDR: %w", diags)
        }

        // Configure oidc flags and set it to KubeAPIServerExtraFlags.
@@ -297,28 +297,45 @@ func createTerraformConfigFile(cfg *config, terraformPath string) error {
        return nil
 }

-func validateNodePrivateCIDRs(cfg *config) ([]string, error) {
+func validateNodePrivateCIDRs(cfg *config) ([]string, hcl.Diagnostics) {
        // If both the deprecated and the new fields are provided then error out.
        if cfg.NodePrivateCIDR != "" && len(cfg.NodePrivateCIDRs) > 0 {
-               return nil, fmt.Errorf("`node_private_cidr` and `node_private_cidrs` cannot be used together," +
-                       " use `node_private_cidrs` only")
+               return nil, hcl.Diagnostics{
+                       &hcl.Diagnostic{
+                               Severity: hcl.DiagError,
+                               Summary:  "`node_private_cidr` and `node_private_cidrs` cannot be used together,",
+                       },
+               }
        }

        // If just the deprecated field is provided then produce a warning.
        if cfg.NodePrivateCIDR != "" {
-               fmt.Printf("Warning: `node_private_cidr` is deprecated, use `node_private_cidrs` instead.\n")
-
-               return []string{cfg.NodePrivateCIDR}, nil
+               return nil, hcl.Diagnostics{
+                       &hcl.Diagnostic{
+                               Severity: hcl.DiagWarning,
+                               Summary:  "`node_private_cidr` is deprecated, use `node_private_cidrs` instead",
+                       },
+               }
        }

        // If there are repeated CIDRs then error out.
        if repeatedCIDR := findDuplicateString(cfg.NodePrivateCIDRs); repeatedCIDR != "" {
-               return nil, fmt.Errorf("multiple entries of node private CIDR: %v", repeatedCIDR)
+               return nil, hcl.Diagnostics{
+                       &hcl.Diagnostic{
+                               Severity: hcl.DiagError,
+                               Summary:  fmt.Sprintf("multiple entries of node private CIDR: %v", repeatedCIDR),
+                       },
+               }
        }

        // If there are no CIDRs provided then error out.
        if len(cfg.NodePrivateCIDRs) == 0 {
-               return nil, fmt.Errorf("neither `node_private_cidr` nor `node_private_cidrs` provided")
+               return nil, hcl.Diagnostics{
+                       &hcl.Diagnostic{
+                               Severity: hcl.DiagError,
+                               Summary:  "neither `node_private_cidr` nor `node_private_cidrs` provided",
+                       },
+               }
        }

        return cfg.NodePrivateCIDRs, nil
@@ -479,12 +496,8 @@ func (c *config) checkValidConfig() hcl.Diagnostics {
                diagnostics = append(diagnostics, diags...)
        }

-       if _, err := validateNodePrivateCIDRs(c); err != nil {
-               diagnostics = append(diagnostics, &hcl.Diagnostic{
-                       Severity: hcl.DiagError,
-                       Summary:  "Node Private CIDR",
-                       Detail:   fmt.Sprintf("Node Private CIDR failed with: %v", err),
-               })
+       if _, diags := validateNodePrivateCIDRs(c); diags != nil {
+               diagnostics = append(diagnostics, diags...)
        }

        return diagnostics

invidian · 2021-02-26T13:25:48Z

Exactly. But as validateNodePrivateCIDRs returns a value, maybe we rename it to nodePrivateCIDRs then? validate... functions usually just return error.

surajssd · 2021-02-26T13:43:43Z

@iaguis incorporated, PTAL.

iaguis · 2021-02-26T14:27:03Z

Exactly. But as validateNodePrivateCIDRs returns a value, maybe we rename it to nodePrivateCIDRs then? validate... functions usually just return error.

Good point, let's name it resolveNodePrivateCIDRs. After that LGTM.

pkg/platform/packet/packet_test.go

- Mark `node_private_cidr` as deprecated. - This is added to support multi-facility worker pools. Different facility in an EM project can have different private node CIDR, hence allow user to provide multiple CIDRs. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

We use octal of format `0o700` in `lokomotive/pkg/terraform_test` package. Without this change the tests fail with following error on go1.16: ``` octal literals requires go1.13 or later (-lang was set to go1.12; check go.mod) ``` It needs go1.13 or later mentioned in `go.mod`. This commit updates the go version in `go.mod`. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

iaguis

lgtm

Dismissing my review

The review was addressed.

invidian suggested changes Feb 3, 2021

View reviewed changes

docs/configuration-reference/platforms/packet.md Outdated Show resolved Hide resolved

surajssd force-pushed the surajssd/add-worker-pool-region branch from 46c6079 to 8978273 Compare February 4, 2021 07:50

surajssd marked this pull request as ready for review February 4, 2021 07:50

surajssd requested review from invidian and knrt10 February 4, 2021 07:50

surajssd mentioned this pull request Feb 4, 2021

docs: How to add worker pool to separate facility #1361

Merged

surajssd force-pushed the surajssd/add-worker-pool-region branch from af0b0b0 to 0c71812 Compare February 5, 2021 09:41

knrt10 previously requested changes Feb 5, 2021

View reviewed changes

docs/configuration-reference/platforms/packet.md Outdated Show resolved Hide resolved

assets/terraform-modules/packet/flatcar-linux/kubernetes/variables.tf Outdated Show resolved Hide resolved

surajssd force-pushed the surajssd/add-worker-pool-region branch from 0c71812 to 309858e Compare February 5, 2021 10:51

surajssd requested a review from knrt10 February 5, 2021 10:56

surajssd force-pushed the surajssd/add-worker-pool-region branch from 309858e to ad04e7e Compare February 5, 2021 10:56

invidian reviewed Feb 5, 2021

View reviewed changes

pkg/platform/packet/packet.go Show resolved Hide resolved

pkg/platform/packet/packet.go Outdated Show resolved Hide resolved

assets/terraform-modules/packet/flatcar-linux/kubernetes/bootkube.tf Outdated Show resolved Hide resolved

knrt10 changed the title ~~packet: Add worker pool specific facility param~~ EM: Add worker pool specific facility param Feb 8, 2021

surajssd force-pushed the surajssd/add-worker-pool-region branch 2 times, most recently from 514635a to 8907cc9 Compare February 9, 2021 07:35

surajssd requested review from invidian and iaguis February 9, 2021 07:36

invidian suggested changes Feb 9, 2021

View reviewed changes

surajssd force-pushed the surajssd/add-worker-pool-region branch 2 times, most recently from da88f55 to 0006ac2 Compare February 10, 2021 11:00

surajssd requested a review from invidian February 10, 2021 11:00

invidian suggested changes Feb 11, 2021

View reviewed changes

iaguis suggested changes Feb 18, 2021

View reviewed changes

pkg/platform/packet/packet.go Outdated Show resolved Hide resolved

pkg/platform/packet/packet.go Outdated Show resolved Hide resolved

pkg/platform/packet/packet.go Outdated Show resolved Hide resolved

pkg/platform/packet/packet_test.go Outdated Show resolved Hide resolved

surajssd force-pushed the surajssd/add-worker-pool-region branch from 0006ac2 to ccfe62b Compare February 19, 2021 15:36

surajssd requested a review from iaguis February 19, 2021 15:38

surajssd force-pushed the surajssd/add-worker-pool-region branch 3 times, most recently from 34b9e3c to f33a370 Compare February 23, 2021 13:45

surajssd requested a review from invidian February 23, 2021 15:59

invidian reviewed Feb 23, 2021

View reviewed changes

pkg/platform/packet/packet.go Outdated Show resolved Hide resolved

pkg/platform/packet/packet.go Show resolved Hide resolved

pkg/platform/packet/packet.go Outdated Show resolved Hide resolved

pkg/platform/packet/packet_test.go Outdated Show resolved Hide resolved

surajssd added this to the v0.7.0 milestone Feb 24, 2021

surajssd added 4 commits February 26, 2021 12:15

EM quickstarts: Remove whitespaces

43a38cb

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

baremetal docs: Remove the EM related variables

9a83af1

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

EM: Use controller private IP for calico IP detection

a935ad1

For calico's node ip auto detection use first controller node's private IP instead of node private CIDR. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

surajssd force-pushed the surajssd/add-worker-pool-region branch 2 times, most recently from de00bfa to d9b32f3 Compare February 26, 2021 08:49

iaguis suggested changes Feb 26, 2021

View reviewed changes

surajssd force-pushed the surajssd/add-worker-pool-region branch from d9b32f3 to a7fa225 Compare February 26, 2021 13:43

surajssd force-pushed the surajssd/add-worker-pool-region branch from a7fa225 to 4843f45 Compare March 1, 2021 08:15

invidian reviewed Mar 1, 2021

View reviewed changes

pkg/platform/packet/packet_test.go Outdated Show resolved Hide resolved

surajssd added 3 commits March 1, 2021 16:10

Fix terraform formatting

869b880

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

surajssd force-pushed the surajssd/add-worker-pool-region branch from 4843f45 to da95497 Compare March 1, 2021 10:41

iaguis approved these changes Mar 1, 2021

View reviewed changes

surajssd merged commit 7d39863 into master Mar 1, 2021

surajssd deleted the surajssd/add-worker-pool-region branch March 1, 2021 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EM: Add worker pool specific facility param #1359

EM: Add worker pool specific facility param #1359

surajssd commented Feb 3, 2021

invidian left a comment

surajssd commented Feb 4, 2021

surajssd commented Feb 4, 2021

invidian commented Feb 4, 2021

surajssd commented Feb 5, 2021

knrt10 left a comment •

edited

Loading

surajssd commented Feb 9, 2021

invidian left a comment

iaguis left a comment

invidian commented Feb 26, 2021

iaguis commented Feb 26, 2021

invidian commented Feb 26, 2021

surajssd commented Feb 26, 2021

iaguis commented Feb 26, 2021

iaguis left a comment

EM: Add worker pool specific facility param #1359

EM: Add worker pool specific facility param #1359

Conversation

surajssd commented Feb 3, 2021

invidian left a comment

Choose a reason for hiding this comment

surajssd commented Feb 4, 2021

surajssd commented Feb 4, 2021

invidian commented Feb 4, 2021

surajssd commented Feb 5, 2021

knrt10 left a comment • edited Loading

Choose a reason for hiding this comment

surajssd commented Feb 9, 2021

invidian left a comment

Choose a reason for hiding this comment

iaguis left a comment

Choose a reason for hiding this comment

invidian commented Feb 26, 2021

iaguis commented Feb 26, 2021

invidian commented Feb 26, 2021

surajssd commented Feb 26, 2021

iaguis commented Feb 26, 2021

iaguis left a comment

Choose a reason for hiding this comment

knrt10 left a comment •

edited

Loading