Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
JoaoJandre closed pull request #8765: Restart VPC with clean-up not applying all LB rules URL: https://github.com/apache/cloudstack/pull/8765 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
blueorangutan commented on PR #8765: URL: https://github.com/apache/cloudstack/pull/8765#issuecomment-2039855970 Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9171 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
DaanHoogland commented on PR #8765: URL: https://github.com/apache/cloudstack/pull/8765#issuecomment-2039710172 @blueorangutan package -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
blueorangutan commented on PR #8765: URL: https://github.com/apache/cloudstack/pull/8765#issuecomment-2039712551 @DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
JoaoJandre commented on PR #8765: URL: https://github.com/apache/cloudstack/pull/8765#issuecomment-2036944121 > This is marked critical, is this a must-have for 4.18.2.0 @JoaoJandre @weizhouapache @DaanHoogland ? Yes, this is a must-have for 4.18.2.0. Now with #8881 we'll have to discuss which solution is best. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
weizhouapache commented on PR #8765: URL: https://github.com/apache/cloudstack/pull/8765#issuecomment-2036686182 > This is marked critical, is this a must-have for 4.18.2.0 @JoaoJandre @weizhouapache @DaanHoogland ? @rohityadavcloud I will create a PR soon -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
DaanHoogland commented on code in PR #8765:
URL: https://github.com/apache/cloudstack/pull/8765#discussion_r1551329091
##
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java:
##
@@ -2578,7 +2579,13 @@ protected void finalizeNetworkRulesForNetwork(final
Commands cmds, final DomainR
}
}
-final List lbs =
_loadBalancerDao.listByNetworkIdAndScheme(guestNetworkId, Scheme.Public);
+List lbs = null;
+Long vpcId = guestNetwork.getVpcId();
+if (vpcId != null) {
+lbs = _loadBalancerDao.listByVpcIdAndScheme(vpcId,
Scheme.Public);
Review Comment:
> @SadiJr if you do not mind, I can create a new PR for it tomorrow
@weizhouapache , would this be an alternative or an addition?
Have you created it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
rohityadavcloud commented on PR #8765: URL: https://github.com/apache/cloudstack/pull/8765#issuecomment-2036665152 This is marked critical, is this a must-have for 4.18.2.0 @JoaoJandre @weizhouapache @DaanHoogland ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
SadiJr commented on code in PR #8765:
URL: https://github.com/apache/cloudstack/pull/8765#discussion_r1540977428
##
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java:
##
@@ -2578,7 +2579,13 @@ protected void finalizeNetworkRulesForNetwork(final
Commands cmds, final DomainR
}
}
-final List lbs =
_loadBalancerDao.listByNetworkIdAndScheme(guestNetworkId, Scheme.Public);
+List lbs = null;
+Long vpcId = guestNetwork.getVpcId();
+if (vpcId != null) {
+lbs = _loadBalancerDao.listByVpcIdAndScheme(vpcId,
Scheme.Public);
Review Comment:
Sure, feel free to create an alternative PR, as it allows the community to
compare the solutions and choose the best one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
weizhouapache commented on code in PR #8765:
URL: https://github.com/apache/cloudstack/pull/8765#discussion_r1532290966
##
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java:
##
@@ -2578,7 +2579,13 @@ protected void finalizeNetworkRulesForNetwork(final
Commands cmds, final DomainR
}
}
-final List lbs =
_loadBalancerDao.listByNetworkIdAndScheme(guestNetworkId, Scheme.Public);
+List lbs = null;
+Long vpcId = guestNetwork.getVpcId();
+if (vpcId != null) {
+lbs = _loadBalancerDao.listByVpcIdAndScheme(vpcId,
Scheme.Public);
Review Comment:
@SadiJr
if you do not mind, I can create a new PR for it tomorrow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
SadiJr commented on code in PR #8765:
URL: https://github.com/apache/cloudstack/pull/8765#discussion_r1532052860
##
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java:
##
@@ -2578,7 +2579,13 @@ protected void finalizeNetworkRulesForNetwork(final
Commands cmds, final DomainR
}
}
-final List lbs =
_loadBalancerDao.listByNetworkIdAndScheme(guestNetworkId, Scheme.Public);
+List lbs = null;
+Long vpcId = guestNetwork.getVpcId();
+if (vpcId != null) {
+lbs = _loadBalancerDao.listByVpcIdAndScheme(vpcId,
Scheme.Public);
Review Comment:
I can understand your point of view, however, considering the criticality
and the time it would take me to implement and test your proposal, I'll
continue with the current implementation and create an issue to optimize such a
process in the future, using your idea. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
weizhouapache commented on code in PR #8765:
URL: https://github.com/apache/cloudstack/pull/8765#discussion_r1528966859
##
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java:
##
@@ -2578,7 +2579,13 @@ protected void finalizeNetworkRulesForNetwork(final
Commands cmds, final DomainR
}
}
-final List lbs =
_loadBalancerDao.listByNetworkIdAndScheme(guestNetworkId, Scheme.Public);
+List lbs = null;
+Long vpcId = guestNetwork.getVpcId();
+if (vpcId != null) {
+lbs = _loadBalancerDao.listByVpcIdAndScheme(vpcId,
Scheme.Public);
Review Comment:
I know what caused the issue and why you made the changes.
What I meant is, the process can be optimized. Only one
LoadBalancerConfigCommand (with LB of all tiers) is needed, maybe after all
other rules are applied.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
SadiJr commented on code in PR #8765:
URL: https://github.com/apache/cloudstack/pull/8765#discussion_r1528951434
##
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java:
##
@@ -2578,7 +2579,13 @@ protected void finalizeNetworkRulesForNetwork(final
Commands cmds, final DomainR
}
}
-final List lbs =
_loadBalancerDao.listByNetworkIdAndScheme(guestNetworkId, Scheme.Public);
+List lbs = null;
+Long vpcId = guestNetwork.getVpcId();
+if (vpcId != null) {
+lbs = _loadBalancerDao.listByVpcIdAndScheme(vpcId,
Scheme.Public);
Review Comment:
Correct, the method is indeed called for each guest network, but the
`LoadBalancerConfigCommand` command, from what I've analyzed in the logs, is
not the same. Instead, for each guest network that calls such a method, only
the load balancer rules for that network are passed in the
`LoadBalancerConfigCommand` command. This is the reason why, in a VPC with
multiple guest networks, each of which has its own load balancer rules, only
the rules of the last implemented network are applied.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
weizhouapache commented on code in PR #8765:
URL: https://github.com/apache/cloudstack/pull/8765#discussion_r1521456790
##
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java:
##
@@ -2578,7 +2579,13 @@ protected void finalizeNetworkRulesForNetwork(final
Commands cmds, final DomainR
}
}
-final List lbs =
_loadBalancerDao.listByNetworkIdAndScheme(guestNetworkId, Scheme.Public);
+List lbs = null;
+Long vpcId = guestNetwork.getVpcId();
+if (vpcId != null) {
+lbs = _loadBalancerDao.listByVpcIdAndScheme(vpcId,
Scheme.Public);
Review Comment:
@SadiJr
the method `finalizeNetworkRulesForNetwork` is called for each guest network
when VPC VR is started
https://github.com/apache/cloudstack/blob/706f3f6ab429be888f982c72e08689b99b9a428e/server/src/main/java/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java#L507-L523
If you search for `StartCommand` in management server logs, you will see
multiple `LoadBalancerConfigCommand` with same parameters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
SadiJr commented on code in PR #8765:
URL: https://github.com/apache/cloudstack/pull/8765#discussion_r1521420585
##
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java:
##
@@ -2578,7 +2579,13 @@ protected void finalizeNetworkRulesForNetwork(final
Commands cmds, final DomainR
}
}
-final List lbs =
_loadBalancerDao.listByNetworkIdAndScheme(guestNetworkId, Scheme.Public);
+List lbs = null;
+Long vpcId = guestNetwork.getVpcId();
+if (vpcId != null) {
+lbs = _loadBalancerDao.listByVpcIdAndScheme(vpcId,
Scheme.Public);
Review Comment:
I'm not sure if I understood exactly what you meant; since LB rules are
being searched for by VPC ID, exceptions should not occur if there are missing
NICs. In the worst case, there will be a rule added to VR of one network that
does not exists.
Regarding applying the rules when all guest networks are added to VR, from
what I saw of the current ACS workflow, the solution I presented corrects the
problem, and I didn't find another part of the code where it can be solved
besides this section. If you think that this solution could be applied to
another section, could you please point to where that would be?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
codecov[bot] commented on PR #8765: URL: https://github.com/apache/cloudstack/pull/8765#issuecomment-1986165905 ## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/8765?src=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) Report Attention: Patch coverage is `0%` with `6 lines` in your changes are missing coverage. Please review. > Project coverage is 13.16%. Comparing base [(`223a9b8`)](https://app.codecov.io/gh/apache/cloudstack/commit/223a9b8031c4f2a7cf1165c960b291ba169996e3?el=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) to head [(`706f3f6`)](https://app.codecov.io/gh/apache/cloudstack/pull/8765?src=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). | [Files](https://app.codecov.io/gh/apache/cloudstack/pull/8765?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Patch % | Lines | |---|---|---| | [...ork/router/VirtualNetworkApplianceManagerImpl.java](https://app.codecov.io/gh/apache/cloudstack/pull/8765?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-c2VydmVyL3NyYy9tYWluL2phdmEvY29tL2Nsb3VkL25ldHdvcmsvcm91dGVyL1ZpcnR1YWxOZXR3b3JrQXBwbGlhbmNlTWFuYWdlckltcGwuamF2YQ==) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/cloudstack/pull/8765?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Additional details and impacted files ```diff @@ Coverage Diff @@ ## 4.18#8765 +/- ## - Coverage 13.16% 13.16% -0.01% Complexity 9205 9205 Files 2724 2724 Lines258149 258153 +4 Branches 4023540236 +1 Hits 3398733987 - Misses 219856 219860 +4 Partials 4306 4306 ``` [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/8765?src=pr=continue_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Restart VPC with clean-up not applying all LB rules [cloudstack]
weizhouapache commented on code in PR #8765:
URL: https://github.com/apache/cloudstack/pull/8765#discussion_r1518051570
##
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java:
##
@@ -2578,7 +2579,13 @@ protected void finalizeNetworkRulesForNetwork(final
Commands cmds, final DomainR
}
}
-final List lbs =
_loadBalancerDao.listByNetworkIdAndScheme(guestNetworkId, Scheme.Public);
+List lbs = null;
+Long vpcId = guestNetwork.getVpcId();
+if (vpcId != null) {
+lbs = _loadBalancerDao.listByVpcIdAndScheme(vpcId,
Scheme.Public);
Review Comment:
LB rules will be applied multiple times (=number of vpc tiers). There might
be exceptions as some guest nics are missing.
It can be applied when all guest nics are added to the VPC VR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
