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

Support F5 partition path #13391

Merged
merged 3 commits into from
Apr 5, 2017
Merged

Conversation

rajatchopra
Copy link
Contributor

F5 router partition path changes:
o Use fully qualified names so that there is no defaulting to /Common
and need to have all the referenced objects in the same partition,
otherwise F5 has reference errors across partitions.
o Fix policy partition path + rework and ensure we check the vserver
which is inside the partition we are configured in.
o Comment re: delete errors.
o Bug fixes.
o F5 unit test changes for supporting partition paths.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1245243

  o Use fully qualified names so that there is no defaulting to /Common
    and need to have all the referenced objects in the same partition,
    otherwise F5 has reference errors across partitions.
  o Fix policy partition path + rework and ensure we check the vserver
    which is inside the partition we are configured in.
  o Comment re: delete errors.
  o Bug fixes.
  o F5 unit test changes for supporting partition paths.
@rajatchopra
Copy link
Contributor Author

[test]

@rajatchopra
Copy link
Contributor Author

@knobunc @eparis FYI

@eparis
Copy link
Member

eparis commented Mar 15, 2017

[test] #12007

@rajatchopra
Copy link
Contributor Author

Basic testing is successful
@openshift/networking Review needed

Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@pravisankar pravisankar left a comment

Choose a reason for hiding this comment

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

F5 integration tests need to be fixed.
Added some comments, Rest of the changes looks good.

@@ -532,7 +576,7 @@ func (f5 *f5LTM) ensureVxLANTunnel() error {
AddressSource: "from-user",
Floating: "disabled",
InheritedTrafficGroup: "false",
TrafficGroup: path.Join(f5.partitionPath, "traffic-group-local-only"),
TrafficGroup: path.Join("/Common", "traffic-group-local-only"), // Traffic group is global
Unit: 0,
Vlan: path.Join(f5.partitionPath, F5VxLANTunnelName),

Choose a reason for hiding this comment

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

Vlan is local per partition or is it global similar to traffic-group?

@@ -727,8 +781,9 @@ func (f5 *f5LTM) ensureIRuleExists(iRuleName, iRule string) error {
iRulesUrl := fmt.Sprintf("https://%s/mgmt/tm/ltm/rule", f5.host)

iRulePayload := f5IRule{
Name: iRuleName,
Code: iRule,
Name: iRuleName,

Choose a reason for hiding this comment

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

iRuleName or f5.iControlUriResourceId(iRuleName)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will confirm with F5 team, but this seems to work so far because the Partition field is also part of the payload

Monitor: "min 1 of /Common/http /Common/https",
Name: poolname,
Mode: "round-robin",
Monitor: "min 1 of /Common/http /Common/https",

Choose a reason for hiding this comment

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

fmt.Sprintf("min 1 of /%s/http /%s/https", f5.partionPath, f5.partionPath)?

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot be sure of http/https monitors being present in the given partition. It is safe to re-use the common monitor just as we re-use TrafficGroup. Good point though, in ideal case we would want this to exist and probably we can write code to ensure the monitors' presence.
Will add a comment about this.

Copy link

@cpganderton cpganderton Apr 4, 2017

Choose a reason for hiding this comment

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

Agree with @rajatchopra, though long term it should be (as @Miciah hinted ) a configurable option, given that the default LTM traffic monitors don't work properly for the atomic registry console and the workaround right now is to manually change the traffic monitor used for the pool for the registry console (given that /Common/http and /Common/https are read-only objects in LTM)

@@ -1187,7 +1243,7 @@ func (f5 *f5LTM) getRoutes(policyname string) (map[string]bool, error) {
}

url := fmt.Sprintf("https://%s/mgmt/tm/ltm/policy/%s/rules",
f5.host, policyname)
f5.host, f5.iControlUriResourceId(policyname))

Choose a reason for hiding this comment

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

https://github.com/openshift/origin/pull/13391/files#diff-997442d3bc5e2bf387c3840016b5c335R1261

If there are two partitions with same policy name, then f5.routes[policyname] will overwrite the former. Do we expect this case? If yes, then f5.routes[f5.iControlUriResourceId(policyname)] = routes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each instance of the controller pod will run with only one given partition. All internal data structures need not bother about multiple partitions.

@ramr
Copy link
Contributor

ramr commented Apr 3, 2017

@rajatchopra lemme know if you want another set of eyes to take a look. These days I tend to shy away for PRs that have more than a few lines of change!! ;^)

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 6ee3a7e

Copy link
Contributor Author

@rajatchopra rajatchopra left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback @pravisankar
I have fixed the tests. Will add the comment and hopefully we can get this in. Thanks.

@@ -727,8 +781,9 @@ func (f5 *f5LTM) ensureIRuleExists(iRuleName, iRule string) error {
iRulesUrl := fmt.Sprintf("https://%s/mgmt/tm/ltm/rule", f5.host)

iRulePayload := f5IRule{
Name: iRuleName,
Code: iRule,
Name: iRuleName,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will confirm with F5 team, but this seems to work so far because the Partition field is also part of the payload

Monitor: "min 1 of /Common/http /Common/https",
Name: poolname,
Mode: "round-robin",
Monitor: "min 1 of /Common/http /Common/https",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot be sure of http/https monitors being present in the given partition. It is safe to re-use the common monitor just as we re-use TrafficGroup. Good point though, in ideal case we would want this to exist and probably we can write code to ensure the monitors' presence.
Will add a comment about this.

@@ -1187,7 +1243,7 @@ func (f5 *f5LTM) getRoutes(policyname string) (map[string]bool, error) {
}

url := fmt.Sprintf("https://%s/mgmt/tm/ltm/policy/%s/rules",
f5.host, policyname)
f5.host, f5.iControlUriResourceId(policyname))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each instance of the controller pod will run with only one given partition. All internal data structures need not bother about multiple partitions.

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/539/) (Base Commit: 3b82ca6)

@rajatchopra
Copy link
Contributor Author

@knobunc I think this is ready for merge

@rajatchopra
Copy link
Contributor Author

lemme know if you want another set of eyes to take a look

@ramr Thanks. I am saving this offer for a real rainy day :)

@knobunc
Copy link
Contributor

knobunc commented Apr 4, 2017

[merge]

@rajatchopra
Copy link
Contributor Author

flake #13606
re [merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 6ee3a7e

@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 5, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/247/) (Base Commit: b39b0ed) (Image: devenv-rhel7_6117)

@openshift-bot openshift-bot merged commit 83a6089 into openshift:master Apr 5, 2017
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.

None yet

7 participants