You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2022/05/25 19:40:59 UTC
[GitHub] [trafficcontrol] traeak opened a new pull request, #6859: T3c parent inner
traeak opened a new pull request, #6859:
URL: https://github.com/apache/trafficcontrol/pull/6859
<!--
Thank you for contributing! Please be sure to read our contribution guidelines: https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md
If this closes or relates to an existing issue, please reference it using one of the following:
Closes: #ISSUE
Related: #ISSUE
If this PR fixes a security vulnerability, DO NOT submit! Instead, contact
the Apache Traffic Control Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://apache.org/security regarding vulnerability disclosure.
-->
This PR adds the ability to add simple/unavailable retries and status codes to parent.config lines at the first an inner tiers for specific delivery services.
This introduces support for example:
- first.max_unavailable_server_retries=1
- inner.max_unavailable_server_retries=1
- last.max_unavailable_server_retries=1
mso. and <null> prefix should be considered deprecated.
For "last" priority order is: `last.`, <null>, then `mso.`
<!-- **^ Add meaningful description above** --><hr/>
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this PR.
Feel free to add the name of a tool or script that is affected but not on the list.
-->
- Documentation
- Traffic Control Cache Config (`t3c`, formerly ORT)
- [x] This PR has documentation <!-- If not, please delete this text and explain why this PR does not need documentation. -->
- [x] This PR has a CHANGELOG.md entry <!-- A fix for a bug from an ATC release, an improvement, or a new feature should have a changelog entry. -->
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a diff in pull request #6859: T3c add parent retry params for first./inner./last.
Posted by GitBox <gi...@apache.org>.
rob05c commented on code in PR #6859:
URL: https://github.com/apache/trafficcontrol/pull/6859#discussion_r903097327
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -802,32 +837,100 @@ type originURI struct {
const deliveryServicesAllParentsKey = "all_parents"
type parentDSParams struct {
- Algorithm ParentAbstractionServiceRetryPolicy
+ Algorithm ParentAbstractionServiceRetryPolicy
+ QueryStringHandling string
+
+ HasRetryParams bool
ParentRetry string
- UnavailableServerRetryResponses string
MaxSimpleRetries string
MaxUnavailableServerRetries string
- QueryStringHandling string
+ SimpleServerRetryResponses string
+ UnavailableServerRetryResponses string
TryAllPrimariesBeforeSecondary bool
- UsePeering bool
- MergeGroups []string
+
+ UsePeering bool
+ MergeGroups []string
+}
+
+func (dsp *parentDSParams) FillParentRetries(keys ParentConfigRetryKeys, dsParams map[string]string, dsid string) (bool, []string) {
+ var warnings []string
+ hasValues := false
+
+ if v, ok := dsParams[keys.Algorithm]; ok && strings.TrimSpace(v) != "" {
+ policy := ParentSelectAlgorithmToParentAbstractionServiceRetryPolicy(v)
+ if policy != ParentAbstractionServiceRetryPolicyInvalid {
+ dsp.Algorithm = policy
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.Algorithm+" parameter '"+v+"', not using!")
+ }
+ }
+
+ if v, ok := dsParams[keys.ParentRetry]; ok {
+ dsp.ParentRetry = v
+ hasValues = true
+ }
+
+ if v, ok := dsParams[keys.MaxSimpleRetries]; ok {
+ dsp.MaxSimpleRetries = v
+ hasValues = true
+ }
+ if v, ok := dsParams[keys.MaxUnavailableRetries]; ok {
+ dsp.MaxUnavailableServerRetries = v
+ hasValues = true
+ }
+
+ if v, ok := dsParams[keys.SimpleRetryResponses]; ok {
+ if unavailableServerRetryResponsesValid(v) {
+ dsp.SimpleServerRetryResponses = v
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.SimpleRetryResponses+" parameter '"+v+"', not using!")
+ }
+ }
+
+ if v, ok := dsParams[keys.UnavailableRetryResponses]; ok {
+ if unavailableServerRetryResponsesValid(v) {
+ dsp.UnavailableServerRetryResponses = v
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.UnavailableRetryResponses+" parameter '"+v+"', not using!")
+ }
+ }
+
+ // Unfortunately this may be s
Review Comment:
What is s?
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -386,214 +403,231 @@ func makeParentDotConfigData(
continue
}
- if txt != nil { // will be nil with no error if this server isn't in the Topology, or if it doesn't have the Required Capabilities
- parentAbstraction.Services = append(parentAbstraction.Services, txt)
+ if pasvc != nil { // will be nil with no error if this server isn't in the Topology, or if it doesn't have the Required Capabilities
+ parentAbstraction.Services = append(parentAbstraction.Services, pasvc)
}
- } else if cacheIsTopLevel {
- parentQStr := false
- if dsParams.QueryStringHandling == "" && dsParams.Algorithm == tc.AlgorithmConsistentHash && ds.QStringIgnore != nil && tc.QStringIgnore(*ds.QStringIgnore) == tc.QStringIgnoreUseInCacheKeyAndPassUp {
- parentQStr = true
- }
-
- orgFQDNStr := *ds.OrgServerFQDN
- // if this cache isn't the last tier, i.e. we're not going to the origin, use http not https
- if isLastCacheTier := noTopologyServerIsLastCacheForDS(server, &ds); !isLastCacheTier {
- orgFQDNStr = strings.Replace(orgFQDNStr, `https://`, `http://`, -1)
- }
- orgURI, orgWarns, err := getOriginURI(orgFQDNStr)
- warnings = append(warnings, orgWarns...)
- if err != nil {
- warnings = append(warnings, "DS '"+*ds.XMLID+"' has malformed origin URI: '"+orgFQDNStr+"': skipping!"+err.Error())
- continue
+ } else {
+ isLastCacheTier := noTopologyServerIsLastCacheForDS(server, &ds)
+ serverPlacement := TopologyPlacement{
+ IsLastCacheTier: isLastCacheTier,
+ IsFirstCacheTier: !isLastCacheTier,
Review Comment:
This isn't correct for go-direct DSes. I'm not seeing a "IsFirst" equivalent, we probably need to add one, which is just whether it's an EDGE-type for non-topology:
```
func noTopologyServerIsFirstCacheForDS(server *Server, ds *DeliveryService) bool {
return strings.HasPrefix(server.Type, tc.EdgeTypePrefix)
}
```
Alternatively, at this point it might be worth adding a func to return a `TopologyPlacement` for non-Topology DSes.
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -802,32 +837,100 @@ type originURI struct {
const deliveryServicesAllParentsKey = "all_parents"
type parentDSParams struct {
- Algorithm ParentAbstractionServiceRetryPolicy
+ Algorithm ParentAbstractionServiceRetryPolicy
+ QueryStringHandling string
+
+ HasRetryParams bool
ParentRetry string
- UnavailableServerRetryResponses string
MaxSimpleRetries string
MaxUnavailableServerRetries string
- QueryStringHandling string
+ SimpleServerRetryResponses string
+ UnavailableServerRetryResponses string
TryAllPrimariesBeforeSecondary bool
- UsePeering bool
- MergeGroups []string
+
+ UsePeering bool
+ MergeGroups []string
+}
+
+func (dsp *parentDSParams) FillParentRetries(keys ParentConfigRetryKeys, dsParams map[string]string, dsid string) (bool, []string) {
+ var warnings []string
+ hasValues := false
+
+ if v, ok := dsParams[keys.Algorithm]; ok && strings.TrimSpace(v) != "" {
+ policy := ParentSelectAlgorithmToParentAbstractionServiceRetryPolicy(v)
+ if policy != ParentAbstractionServiceRetryPolicyInvalid {
+ dsp.Algorithm = policy
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.Algorithm+" parameter '"+v+"', not using!")
+ }
+ }
+
+ if v, ok := dsParams[keys.ParentRetry]; ok {
+ dsp.ParentRetry = v
+ hasValues = true
+ }
+
+ if v, ok := dsParams[keys.MaxSimpleRetries]; ok {
+ dsp.MaxSimpleRetries = v
+ hasValues = true
+ }
+ if v, ok := dsParams[keys.MaxUnavailableRetries]; ok {
+ dsp.MaxUnavailableServerRetries = v
+ hasValues = true
+ }
+
+ if v, ok := dsParams[keys.SimpleRetryResponses]; ok {
+ if unavailableServerRetryResponsesValid(v) {
+ dsp.SimpleServerRetryResponses = v
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.SimpleRetryResponses+" parameter '"+v+"', not using!")
+ }
+ }
+
+ if v, ok := dsParams[keys.UnavailableRetryResponses]; ok {
+ if unavailableServerRetryResponsesValid(v) {
+ dsp.UnavailableServerRetryResponses = v
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.UnavailableRetryResponses+" parameter '"+v+"', not using!")
+ }
+ }
+
+ // Unfortunately this may be s
+ if v, ok := dsParams[keys.SecondaryMode]; ok {
+ if v == "false" {
+ dsp.TryAllPrimariesBeforeSecondary = false
+ } else {
+ dsp.TryAllPrimariesBeforeSecondary = true
+ if v != "" {
+ warnings = append(warnings, "DS '"+dsid+"' had Parameter "+keys.SecondaryMode+" which is used if it exists, the value is ignored (unless false) ! Non-empty value '"+v+"' will be ignored!")
+ }
+ }
+ }
+
+ return hasValues, warnings
}
// getDSParams returns the Delivery Service Profile Parameters used in parent.config, and any warnings.
// If Parameters don't exist, defaults are returned. Non-MSO Delivery Services default to no custom retry logic (we should reevaluate that).
// Note these Parameters are only used for MSO for legacy DeliveryServiceServers DeliveryServices.
// Topology DSes use them for all DSes, MSO and non-MSO.
-func getParentDSParams(ds DeliveryService, profileParentConfigParams map[string]map[string]string) (parentDSParams, []string) {
+func getParentDSParams(ds DeliveryService, profileParentConfigParams map[string]map[string]string, serverPlacement TopologyPlacement, isMSO bool) (parentDSParams, []string) {
warnings := []string{}
params := parentDSParams{}
- isMSO := ds.MultiSiteOrigin != nil && *ds.MultiSiteOrigin
- if isMSO {
+
+ // Default values
+ if serverPlacement.IsLastCacheTier && isMSO {
params.Algorithm = ParentConfigDSParamDefaultMSOAlgorithm
params.ParentRetry = ParentConfigDSParamDefaultMSOParentRetry
params.UnavailableServerRetryResponses = ParentConfigDSParamDefaultMSOUnavailableServerRetryResponses
params.MaxSimpleRetries = strconv.Itoa(ParentConfigDSParamDefaultMaxSimpleRetries)
params.MaxUnavailableServerRetries = strconv.Itoa(ParentConfigDSParamDefaultMaxUnavailableServerRetries)
+ params.HasRetryParams = true
Review Comment:
I'm guessing this is true because we're "faking" the params? If so, would you mind adding a comment explaining that? It's not clear to me how and why it can have params by default, when no parameters exist.
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -802,32 +837,100 @@ type originURI struct {
const deliveryServicesAllParentsKey = "all_parents"
type parentDSParams struct {
- Algorithm ParentAbstractionServiceRetryPolicy
+ Algorithm ParentAbstractionServiceRetryPolicy
+ QueryStringHandling string
+
+ HasRetryParams bool
ParentRetry string
- UnavailableServerRetryResponses string
MaxSimpleRetries string
MaxUnavailableServerRetries string
- QueryStringHandling string
+ SimpleServerRetryResponses string
+ UnavailableServerRetryResponses string
TryAllPrimariesBeforeSecondary bool
- UsePeering bool
- MergeGroups []string
+
+ UsePeering bool
+ MergeGroups []string
+}
+
+func (dsp *parentDSParams) FillParentRetries(keys ParentConfigRetryKeys, dsParams map[string]string, dsid string) (bool, []string) {
Review Comment:
Mind adding a function-level GoDoc comment for this? Particularly documenting its return values?
It's possible to figure out from reading the code, but ideally someone shouldn't have to.
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -840,68 +943,50 @@ func getParentDSParams(ds DeliveryService, profileParentConfigParams map[string]
params.UsePeering = true
}
}
+
// the following may be blank, no default
params.QueryStringHandling = dsParams[ParentConfigParamQStringHandling]
params.MergeGroups = strings.Split(dsParams[ParentConfigParamMergeGroups], " ")
- // TODO deprecate & remove "mso." Parameters - there was never a reason to restrict these settings to MSO.
- if isMSO {
- if v, ok := dsParams[ParentConfigParamMSOAlgorithm]; ok && strings.TrimSpace(v) != "" {
- policy := ParentSelectAlgorithmToParentAbstractionServiceRetryPolicy(v)
- if policy != ParentAbstractionServiceRetryPolicyInvalid {
- params.Algorithm = policy
- } else {
- warnings = append(warnings, "DS '"+*ds.XMLID+"' had malformed "+ParentConfigParamMSOAlgorithm+" parameter '"+v+"', not using!")
- }
- }
- if v, ok := dsParams[ParentConfigParamMSOParentRetry]; ok {
- params.ParentRetry = v
- }
- if v, ok := dsParams[ParentConfigParamMSOUnavailableServerRetryResponses]; ok {
- if v != "" && !unavailableServerRetryResponsesValid(v) {
- warnings = append(warnings, "DS '"+*ds.XMLID+"' had malformed "+ParentConfigParamMSOUnavailableServerRetryResponses+" parameter '"+v+"', not using!")
- } else if v != "" {
- params.UnavailableServerRetryResponses = v
- }
- }
- if v, ok := dsParams[ParentConfigParamMSOMaxSimpleRetries]; ok {
- params.MaxSimpleRetries = v
- }
- if v, ok := dsParams[ParentConfigParamMSOMaxUnavailableServerRetries]; ok {
- params.MaxUnavailableServerRetries = v
+ // Secondary Mode should really be done per tier
Review Comment:
I'm not clear on what this comment is saying. Is it saying it's about to be? Or that it isn't, but it should be?
Would you mind clarifying to e.g. `// TODO set Secondary Mode per tier` or `// secondary mode is set per-tier`?
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] traeak commented on a diff in pull request #6859: T3c add parent retry params for first./inner./last.
Posted by GitBox <gi...@apache.org>.
traeak commented on code in PR #6859:
URL: https://github.com/apache/trafficcontrol/pull/6859#discussion_r906110457
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -802,32 +837,100 @@ type originURI struct {
const deliveryServicesAllParentsKey = "all_parents"
type parentDSParams struct {
- Algorithm ParentAbstractionServiceRetryPolicy
+ Algorithm ParentAbstractionServiceRetryPolicy
+ QueryStringHandling string
+
+ HasRetryParams bool
ParentRetry string
- UnavailableServerRetryResponses string
MaxSimpleRetries string
MaxUnavailableServerRetries string
- QueryStringHandling string
+ SimpleServerRetryResponses string
+ UnavailableServerRetryResponses string
TryAllPrimariesBeforeSecondary bool
- UsePeering bool
- MergeGroups []string
+
+ UsePeering bool
+ MergeGroups []string
+}
+
+func (dsp *parentDSParams) FillParentRetries(keys ParentConfigRetryKeys, dsParams map[string]string, dsid string) (bool, []string) {
Review Comment:
thanks for the heads up
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] traeak commented on a diff in pull request #6859: T3c add parent retry params for first./inner./last.
Posted by GitBox <gi...@apache.org>.
traeak commented on code in PR #6859:
URL: https://github.com/apache/trafficcontrol/pull/6859#discussion_r922298134
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -802,32 +837,100 @@ type originURI struct {
const deliveryServicesAllParentsKey = "all_parents"
type parentDSParams struct {
- Algorithm ParentAbstractionServiceRetryPolicy
+ Algorithm ParentAbstractionServiceRetryPolicy
+ QueryStringHandling string
+
+ HasRetryParams bool
ParentRetry string
- UnavailableServerRetryResponses string
MaxSimpleRetries string
MaxUnavailableServerRetries string
- QueryStringHandling string
+ SimpleServerRetryResponses string
+ UnavailableServerRetryResponses string
TryAllPrimariesBeforeSecondary bool
- UsePeering bool
- MergeGroups []string
+
+ UsePeering bool
+ MergeGroups []string
+}
+
+func (dsp *parentDSParams) FillParentRetries(keys ParentConfigRetryKeys, dsParams map[string]string, dsid string) (bool, []string) {
+ var warnings []string
+ hasValues := false
+
+ if v, ok := dsParams[keys.Algorithm]; ok && strings.TrimSpace(v) != "" {
+ policy := ParentSelectAlgorithmToParentAbstractionServiceRetryPolicy(v)
+ if policy != ParentAbstractionServiceRetryPolicyInvalid {
+ dsp.Algorithm = policy
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.Algorithm+" parameter '"+v+"', not using!")
+ }
+ }
+
+ if v, ok := dsParams[keys.ParentRetry]; ok {
+ dsp.ParentRetry = v
+ hasValues = true
+ }
+
+ if v, ok := dsParams[keys.MaxSimpleRetries]; ok {
+ dsp.MaxSimpleRetries = v
+ hasValues = true
+ }
+ if v, ok := dsParams[keys.MaxUnavailableRetries]; ok {
+ dsp.MaxUnavailableServerRetries = v
+ hasValues = true
+ }
+
+ if v, ok := dsParams[keys.SimpleRetryResponses]; ok {
+ if unavailableServerRetryResponsesValid(v) {
+ dsp.SimpleServerRetryResponses = v
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.SimpleRetryResponses+" parameter '"+v+"', not using!")
+ }
+ }
+
+ if v, ok := dsParams[keys.UnavailableRetryResponses]; ok {
+ if unavailableServerRetryResponsesValid(v) {
+ dsp.UnavailableServerRetryResponses = v
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.UnavailableRetryResponses+" parameter '"+v+"', not using!")
+ }
+ }
+
+ // Unfortunately this may be s
+ if v, ok := dsParams[keys.SecondaryMode]; ok {
+ if v == "false" {
+ dsp.TryAllPrimariesBeforeSecondary = false
+ } else {
+ dsp.TryAllPrimariesBeforeSecondary = true
+ if v != "" {
+ warnings = append(warnings, "DS '"+dsid+"' had Parameter "+keys.SecondaryMode+" which is used if it exists, the value is ignored (unless false) ! Non-empty value '"+v+"' will be ignored!")
+ }
+ }
+ }
+
+ return hasValues, warnings
}
// getDSParams returns the Delivery Service Profile Parameters used in parent.config, and any warnings.
// If Parameters don't exist, defaults are returned. Non-MSO Delivery Services default to no custom retry logic (we should reevaluate that).
// Note these Parameters are only used for MSO for legacy DeliveryServiceServers DeliveryServices.
// Topology DSes use them for all DSes, MSO and non-MSO.
-func getParentDSParams(ds DeliveryService, profileParentConfigParams map[string]map[string]string) (parentDSParams, []string) {
+func getParentDSParams(ds DeliveryService, profileParentConfigParams map[string]map[string]string, serverPlacement TopologyPlacement, isMSO bool) (parentDSParams, []string) {
warnings := []string{}
params := parentDSParams{}
- isMSO := ds.MultiSiteOrigin != nil && *ds.MultiSiteOrigin
- if isMSO {
+
+ // Default values
+ if serverPlacement.IsLastCacheTier && isMSO {
params.Algorithm = ParentConfigDSParamDefaultMSOAlgorithm
params.ParentRetry = ParentConfigDSParamDefaultMSOParentRetry
params.UnavailableServerRetryResponses = ParentConfigDSParamDefaultMSOUnavailableServerRetryResponses
params.MaxSimpleRetries = strconv.Itoa(ParentConfigDSParamDefaultMaxSimpleRetries)
params.MaxUnavailableServerRetries = strconv.Itoa(ParentConfigDSParamDefaultMaxUnavailableServerRetries)
+ params.HasRetryParams = true
Review Comment:
Not faking. If there are no overridden parameters then the defaults are taken which means no parameters are specified on the parent.config line.
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] traeak commented on a diff in pull request #6859: T3c add parent retry params for first./inner./last.
Posted by GitBox <gi...@apache.org>.
traeak commented on code in PR #6859:
URL: https://github.com/apache/trafficcontrol/pull/6859#discussion_r922298134
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -802,32 +837,100 @@ type originURI struct {
const deliveryServicesAllParentsKey = "all_parents"
type parentDSParams struct {
- Algorithm ParentAbstractionServiceRetryPolicy
+ Algorithm ParentAbstractionServiceRetryPolicy
+ QueryStringHandling string
+
+ HasRetryParams bool
ParentRetry string
- UnavailableServerRetryResponses string
MaxSimpleRetries string
MaxUnavailableServerRetries string
- QueryStringHandling string
+ SimpleServerRetryResponses string
+ UnavailableServerRetryResponses string
TryAllPrimariesBeforeSecondary bool
- UsePeering bool
- MergeGroups []string
+
+ UsePeering bool
+ MergeGroups []string
+}
+
+func (dsp *parentDSParams) FillParentRetries(keys ParentConfigRetryKeys, dsParams map[string]string, dsid string) (bool, []string) {
+ var warnings []string
+ hasValues := false
+
+ if v, ok := dsParams[keys.Algorithm]; ok && strings.TrimSpace(v) != "" {
+ policy := ParentSelectAlgorithmToParentAbstractionServiceRetryPolicy(v)
+ if policy != ParentAbstractionServiceRetryPolicyInvalid {
+ dsp.Algorithm = policy
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.Algorithm+" parameter '"+v+"', not using!")
+ }
+ }
+
+ if v, ok := dsParams[keys.ParentRetry]; ok {
+ dsp.ParentRetry = v
+ hasValues = true
+ }
+
+ if v, ok := dsParams[keys.MaxSimpleRetries]; ok {
+ dsp.MaxSimpleRetries = v
+ hasValues = true
+ }
+ if v, ok := dsParams[keys.MaxUnavailableRetries]; ok {
+ dsp.MaxUnavailableServerRetries = v
+ hasValues = true
+ }
+
+ if v, ok := dsParams[keys.SimpleRetryResponses]; ok {
+ if unavailableServerRetryResponsesValid(v) {
+ dsp.SimpleServerRetryResponses = v
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.SimpleRetryResponses+" parameter '"+v+"', not using!")
+ }
+ }
+
+ if v, ok := dsParams[keys.UnavailableRetryResponses]; ok {
+ if unavailableServerRetryResponsesValid(v) {
+ dsp.UnavailableServerRetryResponses = v
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.UnavailableRetryResponses+" parameter '"+v+"', not using!")
+ }
+ }
+
+ // Unfortunately this may be s
+ if v, ok := dsParams[keys.SecondaryMode]; ok {
+ if v == "false" {
+ dsp.TryAllPrimariesBeforeSecondary = false
+ } else {
+ dsp.TryAllPrimariesBeforeSecondary = true
+ if v != "" {
+ warnings = append(warnings, "DS '"+dsid+"' had Parameter "+keys.SecondaryMode+" which is used if it exists, the value is ignored (unless false) ! Non-empty value '"+v+"' will be ignored!")
+ }
+ }
+ }
+
+ return hasValues, warnings
}
// getDSParams returns the Delivery Service Profile Parameters used in parent.config, and any warnings.
// If Parameters don't exist, defaults are returned. Non-MSO Delivery Services default to no custom retry logic (we should reevaluate that).
// Note these Parameters are only used for MSO for legacy DeliveryServiceServers DeliveryServices.
// Topology DSes use them for all DSes, MSO and non-MSO.
-func getParentDSParams(ds DeliveryService, profileParentConfigParams map[string]map[string]string) (parentDSParams, []string) {
+func getParentDSParams(ds DeliveryService, profileParentConfigParams map[string]map[string]string, serverPlacement TopologyPlacement, isMSO bool) (parentDSParams, []string) {
warnings := []string{}
params := parentDSParams{}
- isMSO := ds.MultiSiteOrigin != nil && *ds.MultiSiteOrigin
- if isMSO {
+
+ // Default values
+ if serverPlacement.IsLastCacheTier && isMSO {
params.Algorithm = ParentConfigDSParamDefaultMSOAlgorithm
params.ParentRetry = ParentConfigDSParamDefaultMSOParentRetry
params.UnavailableServerRetryResponses = ParentConfigDSParamDefaultMSOUnavailableServerRetryResponses
params.MaxSimpleRetries = strconv.Itoa(ParentConfigDSParamDefaultMaxSimpleRetries)
params.MaxUnavailableServerRetries = strconv.Itoa(ParentConfigDSParamDefaultMaxUnavailableServerRetries)
+ params.HasRetryParams = true
Review Comment:
Not faking. If there are no overridden parameters then the defaults are taken which means no parameters are specified on the parent.config line. Before this PR args aren't allowed for first and inner, now they must be checked.
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] traeak commented on a diff in pull request #6859: T3c add parent retry params for first./inner./last.
Posted by GitBox <gi...@apache.org>.
traeak commented on code in PR #6859:
URL: https://github.com/apache/trafficcontrol/pull/6859#discussion_r922297519
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -840,68 +943,50 @@ func getParentDSParams(ds DeliveryService, profileParentConfigParams map[string]
params.UsePeering = true
}
}
+
// the following may be blank, no default
params.QueryStringHandling = dsParams[ParentConfigParamQStringHandling]
params.MergeGroups = strings.Split(dsParams[ParentConfigParamMergeGroups], " ")
- // TODO deprecate & remove "mso." Parameters - there was never a reason to restrict these settings to MSO.
- if isMSO {
- if v, ok := dsParams[ParentConfigParamMSOAlgorithm]; ok && strings.TrimSpace(v) != "" {
- policy := ParentSelectAlgorithmToParentAbstractionServiceRetryPolicy(v)
- if policy != ParentAbstractionServiceRetryPolicyInvalid {
- params.Algorithm = policy
- } else {
- warnings = append(warnings, "DS '"+*ds.XMLID+"' had malformed "+ParentConfigParamMSOAlgorithm+" parameter '"+v+"', not using!")
- }
- }
- if v, ok := dsParams[ParentConfigParamMSOParentRetry]; ok {
- params.ParentRetry = v
- }
- if v, ok := dsParams[ParentConfigParamMSOUnavailableServerRetryResponses]; ok {
- if v != "" && !unavailableServerRetryResponsesValid(v) {
- warnings = append(warnings, "DS '"+*ds.XMLID+"' had malformed "+ParentConfigParamMSOUnavailableServerRetryResponses+" parameter '"+v+"', not using!")
- } else if v != "" {
- params.UnavailableServerRetryResponses = v
- }
- }
- if v, ok := dsParams[ParentConfigParamMSOMaxSimpleRetries]; ok {
- params.MaxSimpleRetries = v
- }
- if v, ok := dsParams[ParentConfigParamMSOMaxUnavailableServerRetries]; ok {
- params.MaxUnavailableServerRetries = v
+ // Secondary Mode should really be done per tier
Review Comment:
I just removed this particular piece of code since it should just be done per tier.
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] traeak commented on a diff in pull request #6859: T3c add parent retry params for first./inner./last.
Posted by GitBox <gi...@apache.org>.
traeak commented on code in PR #6859:
URL: https://github.com/apache/trafficcontrol/pull/6859#discussion_r922297519
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -840,68 +943,50 @@ func getParentDSParams(ds DeliveryService, profileParentConfigParams map[string]
params.UsePeering = true
}
}
+
// the following may be blank, no default
params.QueryStringHandling = dsParams[ParentConfigParamQStringHandling]
params.MergeGroups = strings.Split(dsParams[ParentConfigParamMergeGroups], " ")
- // TODO deprecate & remove "mso." Parameters - there was never a reason to restrict these settings to MSO.
- if isMSO {
- if v, ok := dsParams[ParentConfigParamMSOAlgorithm]; ok && strings.TrimSpace(v) != "" {
- policy := ParentSelectAlgorithmToParentAbstractionServiceRetryPolicy(v)
- if policy != ParentAbstractionServiceRetryPolicyInvalid {
- params.Algorithm = policy
- } else {
- warnings = append(warnings, "DS '"+*ds.XMLID+"' had malformed "+ParentConfigParamMSOAlgorithm+" parameter '"+v+"', not using!")
- }
- }
- if v, ok := dsParams[ParentConfigParamMSOParentRetry]; ok {
- params.ParentRetry = v
- }
- if v, ok := dsParams[ParentConfigParamMSOUnavailableServerRetryResponses]; ok {
- if v != "" && !unavailableServerRetryResponsesValid(v) {
- warnings = append(warnings, "DS '"+*ds.XMLID+"' had malformed "+ParentConfigParamMSOUnavailableServerRetryResponses+" parameter '"+v+"', not using!")
- } else if v != "" {
- params.UnavailableServerRetryResponses = v
- }
- }
- if v, ok := dsParams[ParentConfigParamMSOMaxSimpleRetries]; ok {
- params.MaxSimpleRetries = v
- }
- if v, ok := dsParams[ParentConfigParamMSOMaxUnavailableServerRetries]; ok {
- params.MaxUnavailableServerRetries = v
+ // Secondary Mode should really be done per tier
Review Comment:
I just removed this particular piece of code since it just needs to be done per tier.
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] traeak commented on a diff in pull request #6859: T3c add parent retry params for first./inner./last.
Posted by GitBox <gi...@apache.org>.
traeak commented on code in PR #6859:
URL: https://github.com/apache/trafficcontrol/pull/6859#discussion_r906110457
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -802,32 +837,100 @@ type originURI struct {
const deliveryServicesAllParentsKey = "all_parents"
type parentDSParams struct {
- Algorithm ParentAbstractionServiceRetryPolicy
+ Algorithm ParentAbstractionServiceRetryPolicy
+ QueryStringHandling string
+
+ HasRetryParams bool
ParentRetry string
- UnavailableServerRetryResponses string
MaxSimpleRetries string
MaxUnavailableServerRetries string
- QueryStringHandling string
+ SimpleServerRetryResponses string
+ UnavailableServerRetryResponses string
TryAllPrimariesBeforeSecondary bool
- UsePeering bool
- MergeGroups []string
+
+ UsePeering bool
+ MergeGroups []string
+}
+
+func (dsp *parentDSParams) FillParentRetries(keys ParentConfigRetryKeys, dsParams map[string]string, dsid string) (bool, []string) {
Review Comment:
thanks for the heads up, should be resolved.
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c merged pull request #6859: T3c add parent retry params for first./inner./last.
Posted by GitBox <gi...@apache.org>.
rob05c merged PR #6859:
URL: https://github.com/apache/trafficcontrol/pull/6859
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] traeak commented on a diff in pull request #6859: T3c add parent retry params for first./inner./last.
Posted by GitBox <gi...@apache.org>.
traeak commented on code in PR #6859:
URL: https://github.com/apache/trafficcontrol/pull/6859#discussion_r906104744
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -386,214 +403,231 @@ func makeParentDotConfigData(
continue
}
- if txt != nil { // will be nil with no error if this server isn't in the Topology, or if it doesn't have the Required Capabilities
- parentAbstraction.Services = append(parentAbstraction.Services, txt)
+ if pasvc != nil { // will be nil with no error if this server isn't in the Topology, or if it doesn't have the Required Capabilities
+ parentAbstraction.Services = append(parentAbstraction.Services, pasvc)
}
- } else if cacheIsTopLevel {
- parentQStr := false
- if dsParams.QueryStringHandling == "" && dsParams.Algorithm == tc.AlgorithmConsistentHash && ds.QStringIgnore != nil && tc.QStringIgnore(*ds.QStringIgnore) == tc.QStringIgnoreUseInCacheKeyAndPassUp {
- parentQStr = true
- }
-
- orgFQDNStr := *ds.OrgServerFQDN
- // if this cache isn't the last tier, i.e. we're not going to the origin, use http not https
- if isLastCacheTier := noTopologyServerIsLastCacheForDS(server, &ds); !isLastCacheTier {
- orgFQDNStr = strings.Replace(orgFQDNStr, `https://`, `http://`, -1)
- }
- orgURI, orgWarns, err := getOriginURI(orgFQDNStr)
- warnings = append(warnings, orgWarns...)
- if err != nil {
- warnings = append(warnings, "DS '"+*ds.XMLID+"' has malformed origin URI: '"+orgFQDNStr+"': skipping!"+err.Error())
- continue
+ } else {
+ isLastCacheTier := noTopologyServerIsLastCacheForDS(server, &ds)
+ serverPlacement := TopologyPlacement{
+ IsLastCacheTier: isLastCacheTier,
+ IsFirstCacheTier: !isLastCacheTier,
Review Comment:
changed the logic so:
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] traeak commented on a diff in pull request #6859: T3c add parent retry params for first./inner./last.
Posted by GitBox <gi...@apache.org>.
traeak commented on code in PR #6859:
URL: https://github.com/apache/trafficcontrol/pull/6859#discussion_r906104744
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -386,214 +403,231 @@ func makeParentDotConfigData(
continue
}
- if txt != nil { // will be nil with no error if this server isn't in the Topology, or if it doesn't have the Required Capabilities
- parentAbstraction.Services = append(parentAbstraction.Services, txt)
+ if pasvc != nil { // will be nil with no error if this server isn't in the Topology, or if it doesn't have the Required Capabilities
+ parentAbstraction.Services = append(parentAbstraction.Services, pasvc)
}
- } else if cacheIsTopLevel {
- parentQStr := false
- if dsParams.QueryStringHandling == "" && dsParams.Algorithm == tc.AlgorithmConsistentHash && ds.QStringIgnore != nil && tc.QStringIgnore(*ds.QStringIgnore) == tc.QStringIgnoreUseInCacheKeyAndPassUp {
- parentQStr = true
- }
-
- orgFQDNStr := *ds.OrgServerFQDN
- // if this cache isn't the last tier, i.e. we're not going to the origin, use http not https
- if isLastCacheTier := noTopologyServerIsLastCacheForDS(server, &ds); !isLastCacheTier {
- orgFQDNStr = strings.Replace(orgFQDNStr, `https://`, `http://`, -1)
- }
- orgURI, orgWarns, err := getOriginURI(orgFQDNStr)
- warnings = append(warnings, orgWarns...)
- if err != nil {
- warnings = append(warnings, "DS '"+*ds.XMLID+"' has malformed origin URI: '"+orgFQDNStr+"': skipping!"+err.Error())
- continue
+ } else {
+ isLastCacheTier := noTopologyServerIsLastCacheForDS(server, &ds)
+ serverPlacement := TopologyPlacement{
+ IsLastCacheTier: isLastCacheTier,
+ IsFirstCacheTier: !isLastCacheTier,
Review Comment:
changed the logic so:
IsFirstCacheTier: !isLastCacheTier || !ds.Type.UsesMidCache(),
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org