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

Jump to bottom

Remove ELK Flow Collector #3738

Merged
merged 2 commits into from May 10, 2022
Merged

Remove ELK Flow Collector #3738

merged 2 commits into from May 10, 2022

Conversation

heanlan
Copy link
Contributor

@heanlan heanlan commented May 6, 2022

This PR removes ELK Flow Collector's related files:

  1. manifests under build/yamls/elk-flow-collector
  2. jenkins CI validation job
  3. quick deployment options in scripts
  4. documentation

Signed-off-by: heanlan hanlan@vmware.com

@heanlan heanlan added the area/flow-visibility Issues or PRs related to flow visibility support in Antrea label May 6, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 6, 2022 •
edited

Codecov Report

Merging #3738 (91b5445) into main (2065919) will decrease coverage by 18.50%.
The diff coverage is 32.57%.

❗ Current head 91b5445 differs from pull request most recent head 8ecde32. Consider uploading reports for the commit 8ecde32 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #3738       +/-   ##
===========================================
- Coverage   64.60%   46.09%   -18.51%     
===========================================
  Files         278      246       -32     
  Lines       39640    35914     -3726     
===========================================
- Hits        25608    16556     -9052     
- Misses      12043    17719     +5676     
+ Partials     1989     1639      -350
Flag Coverage Δ
e2e-tests 46.09% <32.57%> (?)
kind-e2e-tests ?
unit-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/agent/multicast/mcast_controller.go 0.00% <0.00%> (-67.46%) ⬇️
pkg/agent/multicast/mcast_discovery.go 0.00% <0.00%> (-61.91%) ⬇️
pkg/agent/openflow/framework.go 76.23% <0.00%> (-10.77%) ⬇️
pkg/agent/openflow/multicast.go 0.00% <0.00%> (ø)
pkg/agent/openflow/pipeline.go 67.87% <0.00%> (-6.91%) ⬇️
pkg/agent/proxy/proxier.go 46.17% <0.00%> (-16.75%) ⬇️
pkg/querier/querier.go 55.00% <ø> (ø)
pkg/agent/openflow/client.go 65.30% <5.88%> (-4.48%) ⬇️
...nt/apiserver/handlers/serviceexternalip/handler.go 51.85% <51.85%> (ø)
pkg/ovs/openflow/ofctrl_bridge.go 54.31% <60.00%> (-2.61%) ⬇️
... and 180 more

dreamtalen
dreamtalen reviewed May 6, 2022
View reviewed changes
test/e2e/infra/vagrant/provision.sh
Provisions the Vagrant VMs.
--ip-family <v4|v6|dual> Deploy IPv4, IPv6 or dual-stack Kubernetes cluster.
--large Deploy large vagrant VMs with 2 vCPUs and 4096MB memory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could retain this option? Might be useful when deploying other memory-consuming stuff.

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 agree with your point. My concern is currently the option is only used by ELK, and it's easy to add if we need it later. cc @antoninbas for opinions

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to remove it. Less is more :)

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, we also need to remove these lines in Vagrantfile: https://github.com/antrea-io/antrea/blob/main/test/e2e/infra/vagrant/Vagrantfile#L38

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! removed

@heanlan
Copy link
Contributor Author

heanlan commented May 6, 2022

https://github.com/antrea-io/antrea/runs/6326239828?check_suite_focus=true
markdown dead link check consistently fail on three links. I manually verified all those three links work normally.

antoninbas
antoninbas reviewed May 6, 2022
View reviewed changes
docs/network-flow-visibility.md
@@ -983,165 +944,4 @@ by adding the file in the following section:
./hack/generate-manifest-flow-visibility.sh > build/yamls/flow-visibility.yml
```

### ELK Flow Collector (deprecated)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe instead of removing this whole section, keep a disclaimer for a couple of releases. I think the whole section should be replaced with:

### ELK Flow Collector (removed)

**Starting with Antrea v1.7, support for the ELK Flow Collector has been removed.**
Please consider using the [Grafana Flow Collector](...) instead, which is actively maintained.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a disclaimer as suggested, thanks

test/e2e/infra/vagrant/provision.sh
Provisions the Vagrant VMs.
--ip-family <v4|v6|dual> Deploy IPv4, IPv6 or dual-stack Kubernetes cluster.
--large Deploy large vagrant VMs with 2 vCPUs and 4096MB memory.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to remove it. Less is more :)

@antoninbas antoninbas added the action/release-note Indicates a PR that should be included in release notes. label May 6, 2022
@antoninbas antoninbas added this to the Antrea v1.7 release milestone May 6, 2022
antoninbas
antoninbas previously approved these changes May 7, 2022
View reviewed changes
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

@antoninbas
Copy link
Contributor

/test-all

@heanlan
Remove ELK Flow Collector
66cfadd 
This PR removes ELK Flow Collector's related files:

1. manifests under build/yamls/elk-flow-collector
2. jenkins CI validation job
3. quick deployment options in scripts
4. documentation

Signed-off-by: heanlan <hanlan@vmware.com>
@heanlan
Fix dead markdown link in ci/jenkins/README.md
8ecde32 
Signed-off-by: heanlan <hanlan@vmware.com>
@heanlan
Copy link
Contributor Author

heanlan commented May 9, 2022

/test-all

@XinShuYang
Copy link
Contributor

/test-e2e

@heanlan heanlan requested a review from antoninbas May 10, 2022 16:02
@antoninbas
Copy link
Contributor

/skip-network-policy

@antoninbas
Copy link
Contributor

Unrelated CI failures

@antoninbas antoninbas merged commit 45e4745 into antrea-io:main May 10, 2022
@heanlan heanlan deleted the remove-elk branch May 10, 2022 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dreamtalen dreamtalen dreamtalen left review comments

@antoninbas antoninbas antoninbas approved these changes

Assignees
No one assigned
Labels
action/release-note Indicates a PR that should be included in release notes. area/flow-visibility Issues or PRs related to flow visibility support in Antrea
Projects
None yet
Milestone
Antrea v1.7 release
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@heanlan @codecov-commenter @antoninbas @XinShuYang @dreamtalen