Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

feat: azure arc addon #3634

Merged
merged 12 commits into from Aug 4, 2020
Merged

feat: azure arc addon #3634

merged 12 commits into from Aug 4, 2020

Conversation

jadarsie
Copy link
Member

Reason for Change:
New addon to simplify cluster onboarding to Azure Arc for Kubernetes.

Requirements:

Notes:

@codecov
Copy link

codecov bot commented Jul 22, 2020

Codecov Report

Merging #3634 into master will decrease coverage by 0.00%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3634      +/-   ##
==========================================
- Coverage   73.14%   73.14%   -0.01%     
==========================================
  Files         147      147              
  Lines       25165    25220      +55     
==========================================
+ Hits        18408    18448      +40     
- Misses       5623     5638      +15     
  Partials     1134     1134              
Impacted Files Coverage Δ
pkg/api/common/const.go 40.00% <ø> (ø)
pkg/engine/engine.go 85.64% <0.00%> (-0.31%) ⬇️
pkg/engine/templates_generated.go 53.26% <0.00%> (-0.36%) ⬇️
pkg/api/vlabs/validate.go 79.66% <87.50%> (+0.16%) ⬆️
pkg/api/addons.go 97.79% <100.00%> (+0.02%) ⬆️
pkg/api/k8s_versions.go 100.00% <100.00%> (ø)
pkg/engine/artifacts.go 99.14% <100.00%> (+0.01%) ⬆️
pkg/api/common/versions.go 96.84% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98a395c...f222154. Read the comment docs.

@jadarsie
Copy link
Member Author

Labeled as WIP until the onboarding image is hosted in MCR.

@jadarsie
Copy link
Member Author

Labeled as WIP until the onboarding image is hosted in MCR.

Apparently this is not a requirement any more. Removing label.

@jadarsie
Copy link
Member Author

Labeled as WIP until the onboarding image is hosted in MCR.

Apparently this is not a requirement any more. Removing label.

@@ -886,6 +886,32 @@ func getClusterAutoscalerAddonFuncMap(addon api.KubernetesAddon, cs *api.Contain
}
}

func getArcAddonFuncMap(addon api.KubernetesAddon) template.FuncMap {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this template funcmap, we can use getAddonFuncMap, which already has a way to get the config key vals (ContainerConfig)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added ContainerConfigBase64 func

@jackfrancis
Copy link
Member

We'll want to add a default addon data model via pkg/api/addons.go (hopefully the existing addons will provide ample examples)

@jadarsie jadarsie changed the title [wip] feat: azure arc addon feat: azure arc addon Jul 23, 2020
Example:

```json
{
Copy link
Member

Choose a reason for hiding this comment

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

let's enclose this example JSON in something like this:

{
  ...
  "properties": {
    "orchestratorProfile": {
      "kubernetesConfig": {
        "addons": [
          ...,
          <your example addons configuration here>,
          ...
        ]
      }
    },
    ...
}

@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jackfrancis
Copy link
Member

lgtm, thanks @jadarsie!

rg := cli.Account.ResourceGroup
if err := cli.Account.CreateGroupWithRetry(addon.Config["resourceGroup"], addon.Config["location"], 30*time.Second, cli.Config.Timeout); err != nil {
return errors.Wrapf(err, "Error while trying to create Azure Arc resource group: %s", rg)
if err := cli.Account.CreateArcGroupWithRetry(addon.Config["resourceGroup"], addon.Config["location"], 30*time.Second, cli.Config.Timeout); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

@jadarsie why do we need to create a new func for this?

@@ -205,6 +205,8 @@ func (cli *CLIProvisioner) provision() error {
}
cli.Engine = eng

cli.EnsureArcResourceGroup()
Copy link
Member

Choose a reason for hiding this comment

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

O.K., lets add this after L130 above (after we've called CreateGroupWithRetry and received a nil err):

now := fmt.Sprintf("now=%v", time.Now().Unix())
cli.Account.ResourceGroup = r := ResourceGroup{
	Name:     cli.Config.Name,
	Location: cli.Config.Location,
	Tags: map[string]string{
		"now": now,
	},
}

Then we can get rid of the now var in the CreateGroup method, and get rid of the whole "assign ResourceGroup property to account instance" code.

I think that's the easiest path forward. You can then get rid of the new duplicated "create resource group func" and basically do exactly what you were doing before, without the surprise Account.ResourceGroup override. :)

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

/lgtm

@acs-bot
Copy link

acs-bot commented Aug 3, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, jadarsie

The full list of commands accepted by this bot can be found here.

The pull request process is described here

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

@jadarsie jadarsie merged commit f321785 into Azure:master Aug 4, 2020
penggu pushed a commit to penggu/aks-engine that referenced this pull request Oct 28, 2020
* feat: azure arc addon
* renamed addon
* data model
* product name update
* e2e improvements
* examples
* fixed cluster config
* add arc-onboarding to everything.json
* Account.ResourceGroup
* CreateGroup does not set Account.ResourceGroup
* fix
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants