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

Implement Kubernetes service deployer #246

Merged

Conversation

mtojek
Copy link
Contributor

@mtojek mtojek commented Feb 5, 2021

Issue: #239

How to use it:

elastic-package stack up -d
eval "$(elastic-package stack shellinit)"

kind create cluster

cd test/packages/kubernetes
elastic-package test system --data-streams pod -v

TODOs:

  • Check kubectl context == kind-kind
  • Attach kind container to the Elastic stack network
  • Install Elastic Agent in the cluster
  • Install custom definitions if present
  • Test execution: List cluster agent only
  • Uninstall custom definitions
  • CI: install kind and kubectl
  • Modify the pod system test to use the Kubernetes service deployer
  • Update package-spec to support new files
  • Merge and use Support kubernetes service deployer package-spec#130
    - [ ] Update system docs I'll extract this to a separate PR, so we can push the implementation and write docs based on the pushed state.

@mtojek mtojek self-assigned this Feb 5, 2021
@mtojek mtojek changed the title Implement kubernetes service deployer Implement Kubernetes service deployer Feb 5, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Feb 5, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #246 updated

  • Start Time: 2021-02-16T18:33:39.251+0000

  • Duration: 20 min 33 sec

  • Commit: dbbcc1c

Test stats 🧪

Test Results
Failed 0
Passed 292
Skipped 1
Total 293

Trends 🧪

Image of Build Times

Image of Tests

@mtojek
Copy link
Contributor Author

mtojek commented Feb 9, 2021

Thank you for taking a look, Christos!

The problem with https://github.com/elastic/beats/blob/master/deploy/kubernetes/elastic-agent-kubernetes.yaml is that it deploys two agents for node and cluster. I'm not sure if we need two separate instances to test the integration (actually it's about endpoints/API not the actual deployment). Is this the way to go? Always in-pair?
I don't know why does the YAML file defines two instances of agent instead of two separate YAML files (one per agent). In this PR the pod endpoint I used, was available to the agent in cluster scope.

I'd like to stick to the principle that we should test integrations not the deployment model. It's more like an end-to-end testing, which we're not doing here.

I'm happy to adjust the PR to better fit our needs, but I'd like to understand if this model won't meet our requirements - won' be possible to test some Kubernetes resources.

@ChrsMark
Copy link
Member

ChrsMark commented Feb 9, 2021

Well the model of deploying 2 instances was inherited from Metricbeat (https://github.com/elastic/beats/blob/v7.9.0/deploy/kubernetes/metricbeat-kubernetes.yaml).

However, now I'm thinking of it again and it should be ok to test all datastreams with only one single Agent, both manually but also with system tests so you can just by-pass my previous comment and go ahead with your design. Thanks!

}
currentContext := string(bytes.TrimSpace(output))

if currentContext != "kind-kind" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if currentContext != "kind-kind" {
if currentContext != kindContext {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed

}
}

logger.Debugf("attach service container %s (ID: %s) to stack network %s", kindControlPlaneContainerName, controlPlaneContainerID, stackNetwork)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.Debugf("attach service container %s (ID: %s) to stack network %s", kindControlPlaneContainerName, controlPlaneContainerID, stackNetwork)
logger.Debugf("attach kind control plane container %s (ID: %s) to stack network %s", kindControlPlaneContainerName, controlPlaneContainerID, stackNetwork)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I adjusted the comment to the similar form.

Comment on lines 262 to 268
cmd := exec.Command("kubectl", args...)
errOutput := new(bytes.Buffer)
cmd.Stderr = errOutput
if err := cmd.Run(); err != nil {
return errors.Wrapf(err, "kubectl apply failed (stderr=%q)", errOutput.String())
}
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

We are doing something similar in verifyKindContext. Does it make sense to extract this part into its own function for running generic kubectl commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe these two functions are not good candidates to get unified, but this might a good point to extract it to the kind package.

Copy link
Contributor

Choose a reason for hiding this comment

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

++ to kind package. That would be even better!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Introduced kind, kubectl and added some changes to docker package.

@mtojek
Copy link
Contributor Author

mtojek commented Feb 11, 2021

jenkins run the tests please

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM.

This was not an easy service deployer to build, IMO. Very good job at iterating on it and then ultimately arriving at this PR.

@mtojek
Copy link
Contributor Author

mtojek commented Feb 11, 2021

Thank you for reviewing. Let's hold on with merging this PR immediately as we need the changes in Agent to go through first: elastic/beats#24001 . Not sure in which branch it will land finally, in the worst case I will bump the stack version to 7.12.0-SNAPSHOT.

@mtojek
Copy link
Contributor Author

mtojek commented Feb 16, 2021

/test

This reverts commit 764dfd2.
@mtojek
Copy link
Contributor Author

mtojek commented Feb 16, 2021

I assume that the next revision of Elastic Agent will contain the bugfix (default policy selection) and I'll be able to merge this PR.

@mtojek
Copy link
Contributor Author

mtojek commented Feb 16, 2021

/test

@mtojek mtojek marked this pull request as ready for review February 16, 2021 18:54
@mtojek
Copy link
Contributor Author

mtojek commented Feb 16, 2021

The CI job has finally passed. I'll merge this one and work on enabling it in integrations (install kind).

@mtojek mtojek merged commit 98adbcd into elastic:master Feb 16, 2021
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.

4 participants