loop optimisation in bash

[DaanHoogland]

Description

A small optimisation as suggested by the user in #4449 . I'm not convinced this is actually reducing the memory usage as they suggest. the code itself seems rathrer risk-free. let's discuss.

Fixes: #4449

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)

Screenshots (if appropriate):

How Has This Been Tested?

[DaanHoogland]

@GabrielBrascher @weizhouapache @rhtyd @wido let's discuss this. I've seen issues in large isolated nets on startup but I doubt this is going to be a big help. thoughts?

[yadvr]

LGTM, needs testing and maybe new tests that could simulate that scale (1k+ fw rules?)

[wido]

I'm not sure if this fixes it. Not against changing it, but I don't know if this is the root-cause.

[davidjumani]

Code works, LGTM

[davidjumani]

@rhtyd @DaanHoogland Would splitting the file and processing each part sequentially work ?

[DaanHoogland]

@davidjumani I like the idea, do we have a good logical splitting of the file? the problem states that many firewall rules cause the issue , so splitting over functional parts wouldn't solve this.

[davidjumani]

@DaanHoogland Assuming that the rules are in multiple lines, loading all the lines in memory and processing them would kill the VR. Instead splitting the file into smaller ones and reading them could solve the memory issue

[DaanHoogland]

Again @davidjumani I like the idea. What is the criterium? (no of lines, sections some combination of criterea) can we be sure that lines are self contained and do not require context or order of executing and can we guarantee that?

[yadvr]

@blueorangutan package

[blueorangutan]

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2489

[yadvr]

@blueorangutan test

[blueorangutan]

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

[weizhouapache]

@DaanHoogland @rhtyd
I suggest not to merge this pr into 4.15.0.0

[yadvr]

Yes @weizhouapache, it's marked against 4.15.1.0; (I'm) just kicking tests on PRs.

[DaanHoogland]

@DaanHoogland @rhtyd
I suggest not to merge this pr into 4.15.0.0

I agree, and would say let's not merge at all until we have knowledge if this is making a difference at all.

[blueorangutan]

Trillian test result (tid-3346)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36386 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4451-t3346-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_volumes.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 85 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_06_download_detached_volume	Failure	312.64	test_volumes.py

[yadvr]

@DaanHoogland @weizhouapache @wido should we merge this for 4.15.1.0 (targetting Q2)?

[DaanHoogland]

I don't care for this code @rhtyd . I don't know but i don't think it is an improvement/optimisation

[weizhouapache]

I don't care for this code @rhtyd . I don't know but i don't think it is an improvement/optimisation

same to me.

[yadvr]

Considering all I could find, the pipe based thing would be better compared to redirection as it would avoid memory exhaustion; the pipe/buffer synchronisation (https://stackoverflow.com/questions/9553628/piping-and-redirection). Let's merge this in 4.15 and test over next few weeks, we can revisit and revert if necessary.