-
Notifications
You must be signed in to change notification settings - Fork 97
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
Update fluentd to 1.12.3, update json to 2.4.1, bump version to 0.11.0 #447
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
@davidbtucker, do you think this is a safe update or does it need more testing? |
@qingling128 @ekund , can we relax the runtime dependency fluentd fixed at a particular fluentd version? |
Hi @Cryptophobia - The reason we pin dependency versions is to achieve reproducible builds. Any specific features / bugs you are looking for in Fluentd 1.12? What's the impact scope? |
@qingling128 , that is fine and you can version fluentd separately in your build system or when/where you build your containers. There is no reason why your gem, which is a plugin dependency of fluentd, needs to specify a required version for fluentd. For example, your gem is the only one that does not allow us to upgrade past fluentd v1.11.2 here: https://github.com/vmware/kube-fluentd-operator/blob/master/base-image/Gemfile . We have so many plugins, and this is the only one that constrains us from upgrading to v0.12.0 for absolutely no reason. All of the other plugin gems have very relaxed constraints such as this one for example fluent-plugin-systemd: Same for fluent-plugin-mongo.gemspec: |
@Cryptophobia - Hi, thanks for the suggestion. That is another possibility (aka refactor the build infra to move the version pinning away from gem spec). We'll need to evaluate its impact and get it prioritized. Given the current workload on the team's plate, it might not happen in the short term. We could consider upgrading Fluentd version though. Note that we have internal integration and load tests that need to pass for each release. Fluentd version turned out to be one of main causes when CPU / memory regression happened in the past. So we need to go through a set of validation process to upgrade the Fluentd version. |
@qingling128 , okay, we would appreciate it very much! Hopefully, there is not many breaking changes from |
We're investigating some breakages on introducing Any feedback is appreciated. |
@cosmo0920 , are you hitting that particular issue on Alternative to see if |
Yeah, downgrading to v1.11.x vanishes this issue.
We'd like to go Fluentd v1.12 world. Fluentd v1.12.2 will fix almost |
Sounds like |
https://www.fluentd.org/blog/fluentd-v1.12.2-has-been-released
I'm going to include this gem built from my fork using fluentd v1.12.2 for our KFO project. |
9953de0
to
e2c1136
Compare
I had to change the dependency version pinning to this:
|
Also added upgrade of json gem for CVE-2020-10663 to this PR: |
Looks like fluentd maintainers will be releasing I will update this PR once that version is released. |
3acc8f4
to
c25d4ab
Compare
fluentd v1.12.3 has been released! can we get this merged in now? |
fluent-plugin-google-cloud.gemspec
Outdated
@@ -19,14 +19,14 @@ eos | |||
gem.test_files = gem.files.grep(/^(test)/) | |||
gem.require_paths = ['lib'] | |||
|
|||
gem.add_runtime_dependency 'fluentd', '1.11.2' | |||
gem.add_runtime_dependency 'fluentd', '~> 1.12', '>= 1.12.2', '< 1.13' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me kick off an internal validation process for Fluentd 1.12.3
. As mentioned in #447 (comment), the version has to pinned to an exact version for now, e.g.
gem.add_runtime_dependency 'fluentd', '1.12.3'
Please update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, updated the PR to force exact version 1.12.3
. Ideally, can we move away from forcing fluentd to particular version. Otherwise, anytime we have to upgrade KFO we will always have to fork this gem.
fluent-plugin-google-cloud.gemspec
Outdated
gem.add_runtime_dependency 'googleapis-common-protos', '1.3.10' | ||
gem.add_runtime_dependency 'googleauth', '0.9.0' | ||
gem.add_runtime_dependency 'google-api-client', '0.30.8' | ||
gem.add_runtime_dependency 'google-cloud-logging', '1.6.6' | ||
gem.add_runtime_dependency 'google-protobuf', '3.15.8' | ||
gem.add_runtime_dependency 'grpc', '1.31.1' | ||
gem.add_runtime_dependency 'json', '2.2.0' | ||
gem.add_runtime_dependency 'json', '~> 2.3.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which exact version of json
are we looking for here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are looking for any version above 2.3.0 which is not vulnerable. We can try 2.4.1
as that is recommended in the fluentd provided docker images?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- increment gem version for plugin - update to allow fluentd v1.12.3 gem - update json to 2.4.1 for [CVE-2020-10663](https://www.ruby-lang.org/en/news/2020/03/19/json-dos-cve-2020-10663/) Signed-off-by: Anton Ouzounov <aouzounov@vmware.com>
Updated @qingling128 . |
Thanks! Our internal validation process takes a few days to soak to catch any potential memory leaks etc. We should have some result on Monday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you @qingling128 ! Numbers indeed look stable. Will make other PRs in the future when we need to update fluentd version! |
The release was delayed for a bit because we saw some resource usage regression (turned out to be false alarm). The May 12 release candidate is going through the 4-day validation. If everything passes, the release will happen early next week. |
Actually, it turned out there is still a memory regression, and related to this PR. Currently investigating the issue and try to figure out whether it's due to |
@qingling128 , are we going to report this to https://github.com/fluent/fluentd/ repo? Are the other basegem dependencies versions equivalent to these: https://github.com/vmware/kube-fluentd-operator/blob/master/base-image/basegems/Gemfile ? It is possible that it could be a ruby version issue or another base gem that needs to be upgraded for better memory optimization? |
Filed an upstream bug at fluent/fluentd#3389. The ruby version, dep gems are the same. See details in the upstream bug. |
Looks like fluentd v1.13.0 with a few in_tail fixes will release soon. fluent/fluentd#3333 (comment) . Maybe they will get to looking at the memory/cpu issues. We should try to update to v1.13.0 when it goes out and test again. |
We're also planning to fix in_tail fixes on Fluentd v1.12.4. |
Signed-off-by: Anton Ouzounov aouzounov@vmware.com
Hello maintainers! We are using fluentd-plugin-google-cloud as a plugin in the kube-fluentd-operator . We want to be able to migrate to using fluentd v1.12.0 for the benefit of our users, this is the only plugin currently forcing us to use fluentd v1.11.2 . Can we help upgrade? I don't see any breaking changes in the fluentd changelogs that will present problems.