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/09/28 20:26:45 UTC
[GitHub] [trafficcontrol] rob05c opened a new pull request, #7096: Add health client parent health
rob05c opened a new pull request, #7096:
URL: https://github.com/apache/trafficcontrol/pull/7096
Adds parent health to the health client.
This allows caches to log and markdown parent health based on whether the cache can directly request its parents, in addition to the existing Traffic Monitor health.
Intra-datacenter network issues can cause a child cache to be unable to get to its parent, even though externally both are accessible to the Traffic Monitor. When this happens, we implicitly rely on the ATS parent health and markdown system, which is reactive and typically leads to client timeouts before a markdown and retry of a different parent can be completed. Direct parent health provides faster proactive markdown during these intra-datacenter network events.
This also significantly refactors the health client. It was a single thread with a loop to reload config, refresh TO data, get TM health, and markdown. This makes all operations their own goroutine/microthread service, as the single-threaded work loop just wasn't feasible with the size of work for parent health polling.
It adds 3 health mechanisms: L4 health (a TCP syn-ack-rst), L7 health (a successful HTTP response), and a meta-parent poll which polls the parent's own health client parent health and uses a heuristic of unavailable parents on the parent.
All new parent health mechanisms default to disabled, and should be considered experimental.
## Which Traffic Control components are affected by this PR?
- Traffic Control Health Client (tc-health-client)
## What is the best way to verify this PR?
Enable parent health on the health client, observe logs and markdowns
## If this is a bugfix, which Traffic Control versions contained the bug?
Not a bug fix
## PR submission checklist
- [x] This PR has tests <!-- If not, please delete this text and explain why this PR does not need tests. -->
- [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] zrhoffman commented on a diff in pull request #7096: Add health client parent health
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on code in PR #7096:
URL: https://github.com/apache/trafficcontrol/pull/7096#discussion_r1003601203
##########
tc-health-client/config/config_test.go:
##########
@@ -37,7 +37,7 @@ func TestLoadConfig(t *testing.T) {
}
cfg := Cfg{
- TrafficMonitors: make(map[string]bool, 0),
+ // TrafficMonitors: make(map[string]bool, 0),
Review Comment:
Does this commented need to be here?
##########
tc-health-client/sar/ephemeralportholder.go:
##########
@@ -0,0 +1,77 @@
+package sar
+
+/*
+ * 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.
+ */
+
+import (
+ "errors"
+ "net"
+ "strconv"
+)
+
+// EphemeralPortHolder serves 2 purposes: it gets an ephemeral TCP port, and it holds
+// onto the port so the OS doesn't assign it to any other app.
+//
+// It listens on :0 thereby getting a socket on an ephemeral port.
+// It continues listening but never reading from the socket until Close is called.
+type EphemeralPortHolder struct {
+ listener net.Listener
+ port int
+}
+
+// GetAndHoldEphemeralPort gets an ephemeral port, and listens on it to prevent
+// the OS assigning the port to other apps.
+// Close must be called on the returned EphemeralPortHolder to stop listening.
+func GetAndHoldEphemeralPort(addr string) (*EphemeralPortHolder, error) {
+ addr += ":0"
+ listener, err := net.Listen("tcp", addr)
+ if err != nil {
+ return nil, errors.New("listening: " + err.Error())
+ }
+
+ // get the port now, so EphemeralPortHolder.Port() doesn't need to return an error
+
+ listenAddr := listener.Addr().String()
+ ipPort := SplitLast(listenAddr, ":")
+ if len(ipPort) < 1 {
+ return nil, errors.New("malformed addr '" + listenAddr + "', should have been ip:port") // should never happen
+ }
+ portStr := ipPort[1]
+ port, err := strconv.Atoi(portStr)
+ if err != nil {
+ return nil, errors.New("malformed addr '" + listenAddr + "' port was not an integer") // should never happen
+ }
+ if port > 65535 || port < 0 {
Review Comment:
Nit: `65535` can be `math.MaxUint16`
##########
tc-health-client/tmagent/markdownservice.go:
##########
@@ -0,0 +1,484 @@
+package tmagent
+
+/*
+ * 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.
+ */
+
+import (
+ "bytes"
+ "errors"
+ "os/exec"
+ "path/filepath"
+ "strconv"
+ "time"
+
+ "github.com/apache/trafficcontrol/lib/go-log"
+ "github.com/apache/trafficcontrol/lib/go-tc"
+ "github.com/apache/trafficcontrol/lib/go-util"
+ "github.com/apache/trafficcontrol/tc-health-client/config"
+)
+
+// ParentStatus contains the trafficserver 'HostStatus' fields that
+// are necessary to interface with the trafficserver 'traffic_ctl' command.
+type ParentStatus struct {
+ Fqdn string
+ ActiveReason bool
+ LocalReason bool
+ ManualReason bool
+ LastTmPoll int64
+ UnavailablePollCount int
+ MarkUpPollCount int
+}
+
+// used to get the overall parent availablity from the
+// HostStatus markdown reasons. all markdown reasons
+// must be true for a parent to be considered available.
+func (p ParentStatus) available(reasonCode string) bool {
+ rc := false
+
+ switch reasonCode {
+ case "active":
+ rc = p.ActiveReason
+ case "local":
+ rc = p.LocalReason
+ case "manual":
+ rc = p.ManualReason
+ }
+ return rc
+}
+
+// used to log that a parent's status is either UP or
+// DOWN based upon the HostStatus reason codes. to
+// be considered UP, all reason codes must be 'true'.
+func (p ParentStatus) Status() string {
+ if !p.ActiveReason {
+ return "DOWN"
+ } else if !p.LocalReason {
+ return "DOWN"
+ } else if !p.ManualReason {
+ return "DOWN"
+ }
+ return "UP"
+}
+
+type StatusReason int
+
+// these are the HostStatus reason codes used within
+// trafficserver.
+const (
+ ACTIVE StatusReason = iota
+ LOCAL
+ MANUAL
+)
+
+// used for logging a parent's HostStatus reason code
+// setting.
+func (s StatusReason) String() string {
+ switch s {
+ case ACTIVE:
+ return "ACTIVE"
+ case LOCAL:
+ return "LOCAL"
+ case MANUAL:
+ return "MANUAL"
+ }
+ return "UNDEFINED"
+}
+
+// MarkdownServer contains symbols for manipulating a running MarkdownService
+// started by StartMarkdownService.
+type MarkdownServer struct {
+ // Shutdown signals the MarkdownService to stop its goroutines and release all resources.
+ //
+ // It is not buffered. Writes will block until the markdown service is done with any
+ // ongoing markdown operation. Once a write returns, the MarkdownService will begin
+ // shutdown.
+ // TODO: add signal to callers when shutdown has finished.
+ Shutdown chan<- struct{}
+
+ // UpdateHealth signals to the markdown service that health status has changed, and it should process markdowns again.
+ UpdateHealth func()
+}
+
+func StartMarkdownService(pi *ParentInfo, pollInterval time.Duration) *MarkdownServer {
+ doneCh := make(chan struct{})
+ updateCh := make(chan struct{})
+ updateHealthF := func() {
+ select {
+ case updateCh <- struct{}{}:
+ default:
+ }
+ }
+
+ go markdownServicePoll(pi, pollInterval, doneCh, updateCh)
+ return &MarkdownServer{Shutdown: doneCh, UpdateHealth: updateHealthF}
+}
+
+func markdownServicePoll(pi *ParentInfo, pollInterval time.Duration, doneChan <-chan struct{}, updateChan <-chan struct{}) {
+ for {
+ select {
+ case <-doneChan:
+ return
+ default:
+ <-updateChan
+ start := time.Now()
+ doMarkdown(pi)
+ log.Infof("poll-status poll=markdown ms=%v\n", int(time.Since(start)/time.Millisecond))
+ time.Sleep(pollInterval)
+ }
+ }
+}
+
+// decideHealthy is the algorithm for deciding whether a cache is healthy.
+// It takes the cache hostname, and all health results, and returns a single boolean decision.
+func decideHealthy(
+ markdownMethods map[config.HealthMethod]struct{},
+ parentFQDN string,
+ tmHealth *TrafficMonitorHealth,
+ parentHealthL4 *ParentHealth,
+ parentHealthL7 *ParentHealth,
+ parentServiceHealth *ParentServiceHealth,
+) bool {
+ // TODO use hostname:port? Parents can be healthy on one port/service but not another.
+ // The ATS markdown command can't mark down per-port as of this writing, but
+ // we can at least calculate it, to log that data, and be able to easily
+ // mark down host:port when ATS adds that.
+
+ // note the CacheStatuses has v4 and v6 data,
+ // but ATS also can't mark down hosts per IP version yet,
+ // so we just use the generic/old non-IP-versioned IsAvailable field.
+
+ // TODO make decision algorithm/pessimism/ratio/etc configurable
+
+ tmHealthy := (*bool)(nil)
+ l4Healthy := (*bool)(nil)
+ l7Healthy := (*bool)(nil)
+ recursiveHealthy := (*bool)(nil)
+
+ l4Health, hasL4Health := parentHealthL4.ParentHealthPollResults[parentFQDN]
+ l7Health, hasL7Health := parentHealthL7.ParentHealthPollResults[parentFQDN]
+
+ if _, use := markdownMethods[config.HealthMethodTrafficMonitor]; use && tmHealth.CacheStatuses != nil {
+ parentHostName := parseFqdn(parentFQDN)
+ tmHealthy = util.BoolPtr(tmHealth.CacheStatuses[tc.CacheName(parentHostName)].IsAvailable)
+ }
+ if _, use := markdownMethods[config.HealthMethodParentL4]; use && parentHealthL4 != nil && hasL4Health && len(parentHealthL4.ParentHealthPollResults) > 0 {
+ l4Healthy = util.BoolPtr(l4Health.Healthy)
+ }
+ if _, use := markdownMethods[config.HealthMethodParentL7]; use && parentHealthL7 != nil && hasL7Health && len(parentHealthL7.ParentHealthPollResults) > 0 {
+ l7Healthy = util.BoolPtr(l7Health.Healthy)
+ }
+ if _, use := markdownMethods[config.HealthMethodParentService]; use && parentServiceHealth != nil && parentServiceHealth.ParentServiceHealthPollResults != nil {
+ recursiveHealthy = util.BoolPtr(decideRecursiveHealthy(parentServiceHealth.ParentServiceHealthPollResults[parentFQDN].ParentServiceHealth))
+ }
+
+ printbp := func(bp *bool) string {
+ if bp == nil {
+ return "nil"
+ }
+ return strconv.FormatBool(*bp)
+ }
+
+ // TODO wait for all polls to have results, before marking down?
+ // Maybe it doesn't matter for pessimistic health?
+
+ log.Infof("decide-healthy host=%v tm=%v l4=%v l7=%v svc=%v\n", parentFQDN, printbp(tmHealthy), printbp(l4Healthy), printbp(l7Healthy), printbp(recursiveHealthy))
+
+ // nilOrTrue is a helper func that returns true if the given *bool is nil or true.
+ // This is used for pessimistic health, but if the health doesn't exist for the parent
+ // for a particular health type poller, we don't want to mark it unhealthy.
+ nilOrTrue := func(bp *bool) bool {
+ return bp == nil || *bp
+ }
+
+ // pessimistic: if any health mechanism is unhealthy, consider the host unhealthy
+ return nilOrTrue(tmHealthy) && nilOrTrue(l4Healthy) && nilOrTrue(l7Healthy) && nilOrTrue(recursiveHealthy)
+}
+
+// decideRecursiveHealthy is the algorithm for deciding whether a parent is healthy,
+// based on a heuristic of whether it can get to most parents
+// It takes the cache hostname, and all health results, and returns a single boolean decision.
+//
+// Note this is a heuristic because ATS doesn't currently have the ability to mark down
+// a parent hostname for a single remap.
+// If and when ATS has the ability to mark down parents per-remap, we can mark down parents
+// just for remaps whose origins aren't available on that parent.
+func decideRecursiveHealthy(parentServiceHealth *ParentServiceHealth) bool {
+ // TODO get direct vs final parent data from strategies.yaml and parent.config
+ // Because we really want to heuristically check only the final parents (i.e. origins)
+ // are available on our direct parent, not any other origins it has that aren't assigned
+ // to this cache.
+
+ // TODO make configurable
+ const ratioToConsiderHealthy = 0.5
+
+ parentHealthy := map[string]bool{}
+
+ // TODO this is currently pessimistic: if any health mechanism is unhealthy, consider unhealthy.
+ // make health decision configurable.
+
+ if parentServiceHealth == nil || parentServiceHealth.ParentServiceHealthPollResults == nil {
+ log.Warnln("decideRecursiveHealthy got nil parents, returning healthy")
+ return true
+ }
+
+ for parentFQDN, recursiveHealth := range parentServiceHealth.ParentServiceHealthPollResults {
+ if recursiveHealth.ParentHealthL4 != nil && !recursiveHealth.ParentHealthL4.Healthy {
+ parentHealthy[parentFQDN] = false
+ continue
+ }
+ if recursiveHealth.ParentHealthL7 != nil && !recursiveHealth.ParentHealthL7.Healthy {
+ parentHealthy[parentFQDN] = false
+ continue
+ }
+ if !decideRecursiveHealthy(recursiveHealth.ParentServiceHealth.ParentServiceHealthPollResults[parentFQDN].ParentServiceHealth) {
+ parentHealthy[parentFQDN] = false
+ continue
+ }
+ parentHealthy[parentFQDN] = true
+ }
+
+ numParents := 0
+ numParentsHealthy := 0
+ for _, healthy := range parentHealthy {
+ numParents++
+ if healthy {
+ numParentsHealthy++
+ }
+ }
+ healthyRatio := float64(numParentsHealthy) / float64(numParents)
+ return healthyRatio >= ratioToConsiderHealthy
+}
+
+type parent struct {
+ host string
+ fqdn string
+}
+
+// getParentFQDNs returns the FQDN of all parents in all health types.
+// This is necessary, because some health polls might not have parents, so we need to combine them all
+func getParentFQDNs(pi *ParentInfo, tmh *TrafficMonitorHealth, l4h *ParentHealth, l7h *ParentHealth, sh *ParentServiceHealth) []string {
+
+ // put FQDns in a set first, to remove duplicates
+ fqdnSet := map[string]struct{}{}
+
+ for cacheName, _ := range tmh.CacheStatuses {
+ hostName := string(cacheName)
+ fqdn, ok := pi.ParentHostFQDNs.Load(hostName)
+ if !ok {
+ // this should be normal. Many parents in TM won't be parents of this cache
+ continue
+ }
+ fqdnSet[fqdn] = struct{}{}
+ }
+
+ for fqdn, _ := range l4h.ParentHealthPollResults {
+ fqdnSet[fqdn] = struct{}{}
+ }
+ for fqdn, _ := range l7h.ParentHealthPollResults {
+ fqdnSet[fqdn] = struct{}{}
+ }
+ for fqdn, _ := range sh.ParentServiceHealthPollResults {
+ // we don't need recursive health, just the direct parent. We only need direct parents to mark down.
+ fqdnSet[fqdn] = struct{}{}
+ }
+
+ fqdns := make([]string, 0, len(fqdnSet))
+ for fqdn, _ := range fqdnSet {
+ fqdns = append(fqdns, fqdn)
+ }
+ return fqdns
+}
+
+// HealthSafetyRatio is the ratio of unhealthy parents after which no parents will be marked down.
+//
+// This is a safety mechanism: if for any reason most or all parents are marked down, something
+// is seriously wrong, possibly with the health code itself, and therefore don't mark any parents down,
+const HealthSafetyRatio = 0.3 // TODO make configurable?
+
+func doMarkdown(pi *ParentInfo) {
+ cfg := pi.Cfg.Get()
+
+ tmHealth := pi.TrafficMonitorHealth.Get()
+ parentHealthL4 := pi.ParentHealthL4.Get()
+ parentHealthL7 := pi.ParentHealthL7.Get()
+ parentServiceHealth := pi.ParentServiceHealth.Get()
+
+ parentFQDNs := getParentFQDNs(pi, tmHealth, parentHealthL4, parentHealthL7, parentServiceHealth)
+ unhealthyNum := 0
+ newCacheHealth := map[string]bool{} // map[fqdn]healthy
+ for _, fqdn := range parentFQDNs {
+ newAvailable := decideHealthy(pi.MarkdownMethods, fqdn, tmHealth, parentHealthL4, parentHealthL7, parentServiceHealth)
+ if !newAvailable {
+ unhealthyNum++
+ }
+ newCacheHealth[fqdn] = newAvailable
+ }
+
+ unhealthyParentsExceedSafetyRatio := (float64(unhealthyNum) / float64(len(newCacheHealth))) > HealthSafetyRatio
+ log.Infof("markdown Unhealthy parents %v/%v safety ratio %v\n", unhealthyNum, len(newCacheHealth), HealthSafetyRatio)
+ if unhealthyParentsExceedSafetyRatio {
+ log.Errorf("Unhealthy parents %v/%v exceed safety ratio %v!! Marking all parents up!!\n", unhealthyNum, len(newCacheHealth), HealthSafetyRatio)
+ }
+
+ for _, fqdn := range parentFQDNs {
+ isAvailable := tc.IsAvailable{}
+ {
+ hostName := parseFqdn(fqdn)
+ isAvailable = tmHealth.CacheStatuses[tc.CacheName(hostName)]
+ }
+ parentStatus, ok := pi.LoadParentStatus(fqdn)
+ if !ok {
+ continue // TODO warn? error? is this normal?
+ }
+
+ // update the polling time
+ parentStatus.LastTmPoll = tmHealth.PollTime.Unix()
+
+ newAvailable := newCacheHealth[fqdn]
+ if unhealthyParentsExceedSafetyRatio {
+ newAvailable = true
+ }
+ // newAvailable := isAvailable.IsAvailable
+ oldAvailable := parentStatus.available(cfg.ReasonCode)
+ if oldAvailable != newAvailable {
+ // do not mark down if the configuration disables mark downs.
+ if !cfg.EnableActiveMarkdowns && !newAvailable {
+ log.Infof("TM reports that %s is not available and should be marked DOWN but, mark downs are disabled by configuration", fqdn)
+ } else {
+ if newParentStatus, err := markParent(cfg, parentStatus, isAvailable.Status, newAvailable); err != nil {
+ log.Errorln(err.Error())
+ } else {
+ log.Infoln("TM reports that '" + parentStatus.Fqdn + "' is not available so marked DOWN")
+ parentStatus = newParentStatus
+ }
+ }
+ }
+
+ // if the host is available clear the unavailable poll count if not 0.
+ if oldAvailable && newAvailable {
+ if parentStatus.UnavailablePollCount > 0 {
+ log.Debugf("resetting the UnavailablePollCount for '%s' from %d to 0",
+ fqdn, parentStatus.UnavailablePollCount)
+ parentStatus.UnavailablePollCount = 0
+ }
+ }
+
+ {
+ parentHostName := parseFqdn(parentStatus.Fqdn)
+ pi.ParentHostFQDNs.Store(parentHostName, parentStatus.Fqdn)
+ }
+
+ // even if the status wasn't updated, we always need to update the poll time on the ParentStatus
+ pi.StoreParentStatus(parentStatus.Fqdn, parentStatus)
+ }
+
+}
+
+// markParent is used to mark a parent as up or down in the trafficserver HostStatus subsystem.
+// It takes a parent, modifies it by marking available and setting reasons and other data,
+// and returns the modified/marked parent.
+func markParent(cfg *config.Cfg, pv ParentStatus, cacheStatus string, available bool) (ParentStatus, error) {
+ var hostAvailable bool
+
+ hostName := parseFqdn(pv.Fqdn)
+
+ activeReason := pv.ActiveReason
+ localReason := pv.LocalReason
+ unavailablePollCount := pv.UnavailablePollCount
+ markUpPollCount := pv.MarkUpPollCount
+
+ log.Debugf("hostName: %s, UnavailablePollCount: %d, available: %v", hostName, unavailablePollCount, available)
+
+ if !available { // unavailable
+ unavailablePollCount += 1
+ if unavailablePollCount < cfg.UnavailablePollThreshold {
+ log.Infof("TM indicates %s is unavailable but the UnavailablePollThreshold has not been reached", hostName)
+ hostAvailable = true
+ } else {
+ // marking the host down
+ if err := execTrafficCtl(pv.Fqdn, false, cfg.ReasonCode, cfg.TrafficServerBinDir); err != nil {
+ log.Errorln(err)
+ return ParentStatus{}, err
+ }
+
+ hostAvailable = false
+ // reset the poll counts
+ markUpPollCount = 0
+ unavailablePollCount = 0
+ log.Infof("marked parent %s DOWN, cache status was: %s\n", hostName, cacheStatus)
+ }
+ } else { // available
+ // marking the host up
+ markUpPollCount += 1
+ if markUpPollCount < cfg.MarkUpPollThreshold {
+ log.Infof("TM indicates %s is available but the MarkUpPollThreshold has not been reached", hostName)
+ hostAvailable = false
+ } else {
+ if err := execTrafficCtl(pv.Fqdn, true, cfg.ReasonCode, cfg.TrafficServerBinDir); err != nil {
+ log.Errorln(err)
+ return ParentStatus{}, err
+ }
+ hostAvailable = true
+ // reset the poll counts
+ unavailablePollCount = 0
+ markUpPollCount = 0
+ log.Infof("marked parent %s UP, cache status was: %s\n", hostName, cacheStatus)
+ }
+ }
+
+ // update parent info
+ reason := cfg.ReasonCode
+ switch reason {
+ case "active":
+ activeReason = hostAvailable
+ case "local":
+ localReason = hostAvailable
+ }
+ // save updates
+ pv.ActiveReason = activeReason
+ pv.LocalReason = localReason
+ pv.UnavailablePollCount = unavailablePollCount
+ pv.MarkUpPollCount = markUpPollCount
+ // pi.Parents[hostName] = pv
Review Comment:
Should this commented line be here?
--
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 #7096: Add health client parent health
Posted by GitBox <gi...@apache.org>.
rob05c commented on code in PR #7096:
URL: https://github.com/apache/trafficcontrol/pull/7096#discussion_r1003764401
##########
tc-health-client/tmagent/markdownservice.go:
##########
@@ -0,0 +1,484 @@
+package tmagent
+
+/*
+ * 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.
+ */
+
+import (
+ "bytes"
+ "errors"
+ "os/exec"
+ "path/filepath"
+ "strconv"
+ "time"
+
+ "github.com/apache/trafficcontrol/lib/go-log"
+ "github.com/apache/trafficcontrol/lib/go-tc"
+ "github.com/apache/trafficcontrol/lib/go-util"
+ "github.com/apache/trafficcontrol/tc-health-client/config"
+)
+
+// ParentStatus contains the trafficserver 'HostStatus' fields that
+// are necessary to interface with the trafficserver 'traffic_ctl' command.
+type ParentStatus struct {
+ Fqdn string
+ ActiveReason bool
+ LocalReason bool
+ ManualReason bool
+ LastTmPoll int64
+ UnavailablePollCount int
+ MarkUpPollCount int
+}
+
+// used to get the overall parent availablity from the
+// HostStatus markdown reasons. all markdown reasons
+// must be true for a parent to be considered available.
+func (p ParentStatus) available(reasonCode string) bool {
+ rc := false
+
+ switch reasonCode {
+ case "active":
+ rc = p.ActiveReason
+ case "local":
+ rc = p.LocalReason
+ case "manual":
+ rc = p.ManualReason
+ }
+ return rc
+}
+
+// used to log that a parent's status is either UP or
+// DOWN based upon the HostStatus reason codes. to
+// be considered UP, all reason codes must be 'true'.
+func (p ParentStatus) Status() string {
+ if !p.ActiveReason {
+ return "DOWN"
+ } else if !p.LocalReason {
+ return "DOWN"
+ } else if !p.ManualReason {
+ return "DOWN"
+ }
+ return "UP"
+}
+
+type StatusReason int
+
+// these are the HostStatus reason codes used within
+// trafficserver.
+const (
+ ACTIVE StatusReason = iota
+ LOCAL
+ MANUAL
+)
+
+// used for logging a parent's HostStatus reason code
+// setting.
+func (s StatusReason) String() string {
+ switch s {
+ case ACTIVE:
+ return "ACTIVE"
+ case LOCAL:
+ return "LOCAL"
+ case MANUAL:
+ return "MANUAL"
+ }
+ return "UNDEFINED"
+}
+
+// MarkdownServer contains symbols for manipulating a running MarkdownService
+// started by StartMarkdownService.
+type MarkdownServer struct {
+ // Shutdown signals the MarkdownService to stop its goroutines and release all resources.
+ //
+ // It is not buffered. Writes will block until the markdown service is done with any
+ // ongoing markdown operation. Once a write returns, the MarkdownService will begin
+ // shutdown.
+ // TODO: add signal to callers when shutdown has finished.
+ Shutdown chan<- struct{}
+
+ // UpdateHealth signals to the markdown service that health status has changed, and it should process markdowns again.
+ UpdateHealth func()
+}
+
+func StartMarkdownService(pi *ParentInfo, pollInterval time.Duration) *MarkdownServer {
+ doneCh := make(chan struct{})
+ updateCh := make(chan struct{})
+ updateHealthF := func() {
+ select {
+ case updateCh <- struct{}{}:
+ default:
+ }
+ }
+
+ go markdownServicePoll(pi, pollInterval, doneCh, updateCh)
+ return &MarkdownServer{Shutdown: doneCh, UpdateHealth: updateHealthF}
+}
+
+func markdownServicePoll(pi *ParentInfo, pollInterval time.Duration, doneChan <-chan struct{}, updateChan <-chan struct{}) {
+ for {
+ select {
+ case <-doneChan:
+ return
+ default:
+ <-updateChan
+ start := time.Now()
+ doMarkdown(pi)
+ log.Infof("poll-status poll=markdown ms=%v\n", int(time.Since(start)/time.Millisecond))
+ time.Sleep(pollInterval)
+ }
+ }
+}
+
+// decideHealthy is the algorithm for deciding whether a cache is healthy.
+// It takes the cache hostname, and all health results, and returns a single boolean decision.
+func decideHealthy(
+ markdownMethods map[config.HealthMethod]struct{},
+ parentFQDN string,
+ tmHealth *TrafficMonitorHealth,
+ parentHealthL4 *ParentHealth,
+ parentHealthL7 *ParentHealth,
+ parentServiceHealth *ParentServiceHealth,
+) bool {
+ // TODO use hostname:port? Parents can be healthy on one port/service but not another.
+ // The ATS markdown command can't mark down per-port as of this writing, but
+ // we can at least calculate it, to log that data, and be able to easily
+ // mark down host:port when ATS adds that.
+
+ // note the CacheStatuses has v4 and v6 data,
+ // but ATS also can't mark down hosts per IP version yet,
+ // so we just use the generic/old non-IP-versioned IsAvailable field.
+
+ // TODO make decision algorithm/pessimism/ratio/etc configurable
+
+ tmHealthy := (*bool)(nil)
+ l4Healthy := (*bool)(nil)
+ l7Healthy := (*bool)(nil)
+ recursiveHealthy := (*bool)(nil)
+
+ l4Health, hasL4Health := parentHealthL4.ParentHealthPollResults[parentFQDN]
+ l7Health, hasL7Health := parentHealthL7.ParentHealthPollResults[parentFQDN]
+
+ if _, use := markdownMethods[config.HealthMethodTrafficMonitor]; use && tmHealth.CacheStatuses != nil {
+ parentHostName := parseFqdn(parentFQDN)
+ tmHealthy = util.BoolPtr(tmHealth.CacheStatuses[tc.CacheName(parentHostName)].IsAvailable)
+ }
+ if _, use := markdownMethods[config.HealthMethodParentL4]; use && parentHealthL4 != nil && hasL4Health && len(parentHealthL4.ParentHealthPollResults) > 0 {
+ l4Healthy = util.BoolPtr(l4Health.Healthy)
+ }
+ if _, use := markdownMethods[config.HealthMethodParentL7]; use && parentHealthL7 != nil && hasL7Health && len(parentHealthL7.ParentHealthPollResults) > 0 {
+ l7Healthy = util.BoolPtr(l7Health.Healthy)
+ }
+ if _, use := markdownMethods[config.HealthMethodParentService]; use && parentServiceHealth != nil && parentServiceHealth.ParentServiceHealthPollResults != nil {
+ recursiveHealthy = util.BoolPtr(decideRecursiveHealthy(parentServiceHealth.ParentServiceHealthPollResults[parentFQDN].ParentServiceHealth))
+ }
+
+ printbp := func(bp *bool) string {
+ if bp == nil {
+ return "nil"
+ }
+ return strconv.FormatBool(*bp)
+ }
+
+ // TODO wait for all polls to have results, before marking down?
+ // Maybe it doesn't matter for pessimistic health?
+
+ log.Infof("decide-healthy host=%v tm=%v l4=%v l7=%v svc=%v\n", parentFQDN, printbp(tmHealthy), printbp(l4Healthy), printbp(l7Healthy), printbp(recursiveHealthy))
+
+ // nilOrTrue is a helper func that returns true if the given *bool is nil or true.
+ // This is used for pessimistic health, but if the health doesn't exist for the parent
+ // for a particular health type poller, we don't want to mark it unhealthy.
+ nilOrTrue := func(bp *bool) bool {
+ return bp == nil || *bp
+ }
+
+ // pessimistic: if any health mechanism is unhealthy, consider the host unhealthy
+ return nilOrTrue(tmHealthy) && nilOrTrue(l4Healthy) && nilOrTrue(l7Healthy) && nilOrTrue(recursiveHealthy)
+}
+
+// decideRecursiveHealthy is the algorithm for deciding whether a parent is healthy,
+// based on a heuristic of whether it can get to most parents
+// It takes the cache hostname, and all health results, and returns a single boolean decision.
+//
+// Note this is a heuristic because ATS doesn't currently have the ability to mark down
+// a parent hostname for a single remap.
+// If and when ATS has the ability to mark down parents per-remap, we can mark down parents
+// just for remaps whose origins aren't available on that parent.
+func decideRecursiveHealthy(parentServiceHealth *ParentServiceHealth) bool {
+ // TODO get direct vs final parent data from strategies.yaml and parent.config
+ // Because we really want to heuristically check only the final parents (i.e. origins)
+ // are available on our direct parent, not any other origins it has that aren't assigned
+ // to this cache.
+
+ // TODO make configurable
+ const ratioToConsiderHealthy = 0.5
+
+ parentHealthy := map[string]bool{}
+
+ // TODO this is currently pessimistic: if any health mechanism is unhealthy, consider unhealthy.
+ // make health decision configurable.
+
+ if parentServiceHealth == nil || parentServiceHealth.ParentServiceHealthPollResults == nil {
+ log.Warnln("decideRecursiveHealthy got nil parents, returning healthy")
+ return true
+ }
+
+ for parentFQDN, recursiveHealth := range parentServiceHealth.ParentServiceHealthPollResults {
+ if recursiveHealth.ParentHealthL4 != nil && !recursiveHealth.ParentHealthL4.Healthy {
+ parentHealthy[parentFQDN] = false
+ continue
+ }
+ if recursiveHealth.ParentHealthL7 != nil && !recursiveHealth.ParentHealthL7.Healthy {
+ parentHealthy[parentFQDN] = false
+ continue
+ }
+ if !decideRecursiveHealthy(recursiveHealth.ParentServiceHealth.ParentServiceHealthPollResults[parentFQDN].ParentServiceHealth) {
+ parentHealthy[parentFQDN] = false
+ continue
+ }
+ parentHealthy[parentFQDN] = true
+ }
+
+ numParents := 0
+ numParentsHealthy := 0
+ for _, healthy := range parentHealthy {
+ numParents++
+ if healthy {
+ numParentsHealthy++
+ }
+ }
+ healthyRatio := float64(numParentsHealthy) / float64(numParents)
+ return healthyRatio >= ratioToConsiderHealthy
+}
+
+type parent struct {
+ host string
+ fqdn string
+}
+
+// getParentFQDNs returns the FQDN of all parents in all health types.
+// This is necessary, because some health polls might not have parents, so we need to combine them all
+func getParentFQDNs(pi *ParentInfo, tmh *TrafficMonitorHealth, l4h *ParentHealth, l7h *ParentHealth, sh *ParentServiceHealth) []string {
+
+ // put FQDns in a set first, to remove duplicates
+ fqdnSet := map[string]struct{}{}
+
+ for cacheName, _ := range tmh.CacheStatuses {
+ hostName := string(cacheName)
+ fqdn, ok := pi.ParentHostFQDNs.Load(hostName)
+ if !ok {
+ // this should be normal. Many parents in TM won't be parents of this cache
+ continue
+ }
+ fqdnSet[fqdn] = struct{}{}
+ }
+
+ for fqdn, _ := range l4h.ParentHealthPollResults {
+ fqdnSet[fqdn] = struct{}{}
+ }
+ for fqdn, _ := range l7h.ParentHealthPollResults {
+ fqdnSet[fqdn] = struct{}{}
+ }
+ for fqdn, _ := range sh.ParentServiceHealthPollResults {
+ // we don't need recursive health, just the direct parent. We only need direct parents to mark down.
+ fqdnSet[fqdn] = struct{}{}
+ }
+
+ fqdns := make([]string, 0, len(fqdnSet))
+ for fqdn, _ := range fqdnSet {
+ fqdns = append(fqdns, fqdn)
+ }
+ return fqdns
+}
+
+// HealthSafetyRatio is the ratio of unhealthy parents after which no parents will be marked down.
+//
+// This is a safety mechanism: if for any reason most or all parents are marked down, something
+// is seriously wrong, possibly with the health code itself, and therefore don't mark any parents down,
+const HealthSafetyRatio = 0.3 // TODO make configurable?
+
+func doMarkdown(pi *ParentInfo) {
+ cfg := pi.Cfg.Get()
+
+ tmHealth := pi.TrafficMonitorHealth.Get()
+ parentHealthL4 := pi.ParentHealthL4.Get()
+ parentHealthL7 := pi.ParentHealthL7.Get()
+ parentServiceHealth := pi.ParentServiceHealth.Get()
+
+ parentFQDNs := getParentFQDNs(pi, tmHealth, parentHealthL4, parentHealthL7, parentServiceHealth)
+ unhealthyNum := 0
+ newCacheHealth := map[string]bool{} // map[fqdn]healthy
+ for _, fqdn := range parentFQDNs {
+ newAvailable := decideHealthy(pi.MarkdownMethods, fqdn, tmHealth, parentHealthL4, parentHealthL7, parentServiceHealth)
+ if !newAvailable {
+ unhealthyNum++
+ }
+ newCacheHealth[fqdn] = newAvailable
+ }
+
+ unhealthyParentsExceedSafetyRatio := (float64(unhealthyNum) / float64(len(newCacheHealth))) > HealthSafetyRatio
+ log.Infof("markdown Unhealthy parents %v/%v safety ratio %v\n", unhealthyNum, len(newCacheHealth), HealthSafetyRatio)
+ if unhealthyParentsExceedSafetyRatio {
+ log.Errorf("Unhealthy parents %v/%v exceed safety ratio %v!! Marking all parents up!!\n", unhealthyNum, len(newCacheHealth), HealthSafetyRatio)
+ }
+
+ for _, fqdn := range parentFQDNs {
+ isAvailable := tc.IsAvailable{}
+ {
+ hostName := parseFqdn(fqdn)
+ isAvailable = tmHealth.CacheStatuses[tc.CacheName(hostName)]
+ }
+ parentStatus, ok := pi.LoadParentStatus(fqdn)
+ if !ok {
+ continue // TODO warn? error? is this normal?
+ }
+
+ // update the polling time
+ parentStatus.LastTmPoll = tmHealth.PollTime.Unix()
+
+ newAvailable := newCacheHealth[fqdn]
+ if unhealthyParentsExceedSafetyRatio {
+ newAvailable = true
+ }
+ // newAvailable := isAvailable.IsAvailable
+ oldAvailable := parentStatus.available(cfg.ReasonCode)
+ if oldAvailable != newAvailable {
+ // do not mark down if the configuration disables mark downs.
+ if !cfg.EnableActiveMarkdowns && !newAvailable {
+ log.Infof("TM reports that %s is not available and should be marked DOWN but, mark downs are disabled by configuration", fqdn)
+ } else {
+ if newParentStatus, err := markParent(cfg, parentStatus, isAvailable.Status, newAvailable); err != nil {
+ log.Errorln(err.Error())
+ } else {
+ log.Infoln("TM reports that '" + parentStatus.Fqdn + "' is not available so marked DOWN")
+ parentStatus = newParentStatus
+ }
+ }
+ }
+
+ // if the host is available clear the unavailable poll count if not 0.
+ if oldAvailable && newAvailable {
+ if parentStatus.UnavailablePollCount > 0 {
+ log.Debugf("resetting the UnavailablePollCount for '%s' from %d to 0",
+ fqdn, parentStatus.UnavailablePollCount)
+ parentStatus.UnavailablePollCount = 0
+ }
+ }
+
+ {
+ parentHostName := parseFqdn(parentStatus.Fqdn)
+ pi.ParentHostFQDNs.Store(parentHostName, parentStatus.Fqdn)
+ }
+
+ // even if the status wasn't updated, we always need to update the poll time on the ParentStatus
+ pi.StoreParentStatus(parentStatus.Fqdn, parentStatus)
+ }
+
+}
+
+// markParent is used to mark a parent as up or down in the trafficserver HostStatus subsystem.
+// It takes a parent, modifies it by marking available and setting reasons and other data,
+// and returns the modified/marked parent.
+func markParent(cfg *config.Cfg, pv ParentStatus, cacheStatus string, available bool) (ParentStatus, error) {
+ var hostAvailable bool
+
+ hostName := parseFqdn(pv.Fqdn)
+
+ activeReason := pv.ActiveReason
+ localReason := pv.LocalReason
+ unavailablePollCount := pv.UnavailablePollCount
+ markUpPollCount := pv.MarkUpPollCount
+
+ log.Debugf("hostName: %s, UnavailablePollCount: %d, available: %v", hostName, unavailablePollCount, available)
+
+ if !available { // unavailable
+ unavailablePollCount += 1
+ if unavailablePollCount < cfg.UnavailablePollThreshold {
+ log.Infof("TM indicates %s is unavailable but the UnavailablePollThreshold has not been reached", hostName)
+ hostAvailable = true
+ } else {
+ // marking the host down
+ if err := execTrafficCtl(pv.Fqdn, false, cfg.ReasonCode, cfg.TrafficServerBinDir); err != nil {
+ log.Errorln(err)
+ return ParentStatus{}, err
+ }
+
+ hostAvailable = false
+ // reset the poll counts
+ markUpPollCount = 0
+ unavailablePollCount = 0
+ log.Infof("marked parent %s DOWN, cache status was: %s\n", hostName, cacheStatus)
+ }
+ } else { // available
+ // marking the host up
+ markUpPollCount += 1
+ if markUpPollCount < cfg.MarkUpPollThreshold {
+ log.Infof("TM indicates %s is available but the MarkUpPollThreshold has not been reached", hostName)
+ hostAvailable = false
+ } else {
+ if err := execTrafficCtl(pv.Fqdn, true, cfg.ReasonCode, cfg.TrafficServerBinDir); err != nil {
+ log.Errorln(err)
+ return ParentStatus{}, err
+ }
+ hostAvailable = true
+ // reset the poll counts
+ unavailablePollCount = 0
+ markUpPollCount = 0
+ log.Infof("marked parent %s UP, cache status was: %s\n", hostName, cacheStatus)
+ }
+ }
+
+ // update parent info
+ reason := cfg.ReasonCode
+ switch reason {
+ case "active":
+ activeReason = hostAvailable
+ case "local":
+ localReason = hostAvailable
+ }
+ // save updates
+ pv.ActiveReason = activeReason
+ pv.LocalReason = localReason
+ pv.UnavailablePollCount = unavailablePollCount
+ pv.MarkUpPollCount = markUpPollCount
+ // pi.Parents[hostName] = pv
Review Comment:
No, the func was changed to return parents rather than mutate state. Removed
--
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 #7096: Add health client parent health
Posted by GitBox <gi...@apache.org>.
rob05c commented on code in PR #7096:
URL: https://github.com/apache/trafficcontrol/pull/7096#discussion_r1003764988
##########
tc-health-client/config/config_test.go:
##########
@@ -37,7 +37,7 @@ func TestLoadConfig(t *testing.T) {
}
cfg := Cfg{
- TrafficMonitors: make(map[string]bool, 0),
+ // TrafficMonitors: make(map[string]bool, 0),
Review Comment:
No, the data was moved to a different variable and constructed by default. Removed
--
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] github-code-scanning[bot] commented on a diff in pull request #7096: Add health client parent health
Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #7096:
URL: https://github.com/apache/trafficcontrol/pull/7096#discussion_r982897220
##########
tc-health-client/sar/sar.go:
##########
@@ -0,0 +1,348 @@
+// package sar implements a syn-ack-rst health ping.
+// It sends a TCP SYN, waits for an ACK, then immediately sends an RST to kill the connection.
+// The primary purpose of this is as a health check, to verify the remote host is reachable, and able and willing to respond.
+package sar
+
+/*
+ * 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.
+ */
+
+import (
+ "errors"
+ "fmt"
+ "net"
+ "strings"
+ "sync"
+ "time"
+
+ "github.com/apache/trafficcontrol/lib/go-llog"
+)
+
+// SAR calls SARAddr if host is an IP address, or SARHost if host is an FQDN.
+func SAR(log llog.Log, host string, port int, timeout time.Duration) (time.Duration, error) {
+ log = llog.LibInit(log)
+ if ip := net.ParseIP(host); ip != nil {
+ return SARAddr(log, host, port, timeout)
+ }
+ return SARHost(log, host, port, timeout)
+}
+
+// SARAddr sends a syn-ack-reset to the given addr.
+// The addr must be an IP.
+// On success, the round-trip time to receive the Ack is returned, with a nil error.
+// The addr is a string representation of an IPv4 or IPv6 address.
+// TODO add optional local addr param
+func SARAddr(log llog.Log, addr string, port int, timeout time.Duration) (time.Duration, error) {
+ log = llog.LibInit(log)
+ remoteAddr := net.ParseIP(addr)
+ if remoteAddr == nil {
+ return 0, errors.New("failed to parse addr '" + addr + "' as IP")
+ }
+ if v4 := remoteAddr.To4(); v4 != nil {
+ remoteAddr = v4
+ }
+
+ localAddrStr, err := GetLocalAddr()
+ if err != nil {
+ return 0, errors.New("getting local address: " + err.Error())
+ }
+
+ localAddr := net.ParseIP(localAddrStr)
+ if localAddr == nil {
+ return 0, errors.New("failed to parse local addr '" + localAddrStr + "' as IP")
+ }
+ if v4 := localAddr.To4(); v4 != nil {
+ localAddr = v4
+ }
+
+ ephemeralPortHolder, err := GetAndHoldEphemeralPort(localAddrStr)
+ if err != nil {
+ return 0, errors.New("failed to listen on ephemeral port: " + err.Error())
+ }
+ defer ephemeralPortHolder.Close()
+
+ srcPort := ephemeralPortHolder.Port()
+
+ // TODO implement Initial Sequence Number ISN per RFC9293§3.4.1
+ seqNum := uint32(42)
+ window := 256 * 10
+ destPort := port
+ dataOffset := 5 // because we have no options?
+
+ native := TCPHdrNative{
+ SrcPort: uint16(srcPort),
Review Comment:
## Incorrect conversion between integer types
Incorrect conversion of an integer with architecture-dependent bit size from [strconv.Atoi](1) to a lower bit size type uint16 without an upper bound check.
[Show more details](https://github.com/apache/trafficcontrol/security/code-scanning/227)
--
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 #7096: Add health client parent health
Posted by GitBox <gi...@apache.org>.
rob05c commented on code in PR #7096:
URL: https://github.com/apache/trafficcontrol/pull/7096#discussion_r1003762350
##########
tc-health-client/sar/ephemeralportholder.go:
##########
@@ -0,0 +1,77 @@
+package sar
+
+/*
+ * 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.
+ */
+
+import (
+ "errors"
+ "net"
+ "strconv"
+)
+
+// EphemeralPortHolder serves 2 purposes: it gets an ephemeral TCP port, and it holds
+// onto the port so the OS doesn't assign it to any other app.
+//
+// It listens on :0 thereby getting a socket on an ephemeral port.
+// It continues listening but never reading from the socket until Close is called.
+type EphemeralPortHolder struct {
+ listener net.Listener
+ port int
+}
+
+// GetAndHoldEphemeralPort gets an ephemeral port, and listens on it to prevent
+// the OS assigning the port to other apps.
+// Close must be called on the returned EphemeralPortHolder to stop listening.
+func GetAndHoldEphemeralPort(addr string) (*EphemeralPortHolder, error) {
+ addr += ":0"
+ listener, err := net.Listen("tcp", addr)
+ if err != nil {
+ return nil, errors.New("listening: " + err.Error())
+ }
+
+ // get the port now, so EphemeralPortHolder.Port() doesn't need to return an error
+
+ listenAddr := listener.Addr().String()
+ ipPort := SplitLast(listenAddr, ":")
+ if len(ipPort) < 1 {
+ return nil, errors.New("malformed addr '" + listenAddr + "', should have been ip:port") // should never happen
+ }
+ portStr := ipPort[1]
+ port, err := strconv.Atoi(portStr)
+ if err != nil {
+ return nil, errors.New("malformed addr '" + listenAddr + "' port was not an integer") // should never happen
+ }
+ if port > 65535 || port < 0 {
Review Comment:
The number is technically the same, but "the last valid port number" is semantically different than "the last number a uint16 can store." We could make a constant, but it seems a bit overkill for just a port check, I'd prefer to just keep the literal for now
--
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 pull request #7096: Add health client parent health
Posted by GitBox <gi...@apache.org>.
rob05c commented on PR #7096:
URL: https://github.com/apache/trafficcontrol/pull/7096#issuecomment-1281253181
> One small typo in the README.md
Fixed
--
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] zrhoffman merged pull request #7096: Add health client parent health
Posted by GitBox <gi...@apache.org>.
zrhoffman merged PR #7096:
URL: https://github.com/apache/trafficcontrol/pull/7096
--
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 pull request #7096: Add health client parent health
Posted by GitBox <gi...@apache.org>.
rob05c commented on PR #7096:
URL: https://github.com/apache/trafficcontrol/pull/7096#issuecomment-1261503484
Making not a draft to make Github tests to run. Still really a draft.
--
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] github-code-scanning[bot] commented on a diff in pull request #7096: Add health client parent health
Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #7096:
URL: https://github.com/apache/trafficcontrol/pull/7096#discussion_r982922207
##########
tc-health-client/sar/multisar.go:
##########
@@ -0,0 +1,388 @@
+// package sar implements a syn-ack-rst health ping.
+// It sends a TCP SYN, waits for an ACK, then immediately sends an RST to kill the connection.
+// The primary purpose of this is as a health check, to verify the remote host is reachable, and able and willing to respond.
+package sar
+
+/*
+ * 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.
+ */
+
+import (
+ "errors"
+ "net"
+ "os"
+ "sync"
+ "time"
+
+ "github.com/apache/trafficcontrol/lib/go-llog"
+)
+
+type HostPort struct {
+ Host string
+ Port int
+}
+
+type SARResult struct {
+ Host string
+ Port int
+ RTT time.Duration
+ Err error
+}
+
+// MultiSAR is like SAR for multiple requests.
+// SAR has to listen on a raw IP port without a TCP socket, which is relatively inexpensive for a single request,
+// but expensive for large numbers of requests.
+// MultiSAR uses a single listener on an ephemeral local port for all SAR requests, significantly reducing
+// resource costs.
+func MultiSAR(log llog.Log, hosts []HostPort, timeout time.Duration) ([]SARResult, error) {
+ log = llog.LibInit(log)
+
+ localAddrStr, err := GetLocalAddr()
+ if err != nil {
+ return nil, errors.New("getting local address: " + err.Error())
+ }
+
+ localAddr := net.ParseIP(localAddrStr)
+ if localAddr == nil {
+ return nil, errors.New("failed to parse local addr '" + localAddrStr + "' as IP")
+ }
+ if v4 := localAddr.To4(); v4 != nil {
+ localAddr = v4
+ }
+
+ ephemeralPortHolder, err := GetAndHoldEphemeralPort(localAddrStr)
+ if err != nil {
+ return nil, errors.New("failed to listen on ephemeral port: " + err.Error())
+ }
+ defer ephemeralPortHolder.Close()
+
+ srcPort := ephemeralPortHolder.Port()
+
+ // pre-construct all the packets, so we listen for as little time as possible
+
+ // TODO implement Initial Sequence Number ISN per RFC9293§3.4.1? It might be faster to use the same seq num for all packets
+ seqNum := uint32(42)
+
+ type HostPortPacket struct {
+ HostPort
+ TCPHdr TCPHdr
+ }
+
+ packets := []HostPortPacket{}
+
+ hostAddr := map[string]string{} // map[host]addr - note hosts may be IPs, in which case host and addr will be the same
+ addrHosts := map[string][]string{} // map[addr][]host - note multiple FQDNs may have the same IP
+
+ results := []SARResult{}
+
+ for _, host := range hosts {
+ makeHostErrResult := func(err error) SARResult {
+ return SARResult{
+ Host: host.Host,
+ Port: host.Port,
+ RTT: 0,
+ Err: err,
+ }
+ }
+
+ remoteAddr := net.ParseIP(host.Host)
+ if remoteAddr != nil {
+ // host is IP
+ hostAddr[host.Host] = host.Host
+ addrHosts[host.Host] = append(addrHosts[host.Host], host.Host)
+ } else {
+ // host isn't an IP, assume FQDN
+ addrs, err := net.LookupHost(host.Host)
+ if err != nil {
+ results = append(results, makeHostErrResult(errors.New("lookup up host '"+host.Host+"': "+err.Error())))
+ continue
+ }
+ if len(addrs) == 0 {
+ results = append(results, makeHostErrResult(errors.New("looking up host '"+host.Host+"' succeeded, but no addresses were found.")))
+ continue
+ }
+ remoteAddr = net.ParseIP(addrs[0])
+ if remoteAddr == nil {
+ results = append(results, makeHostErrResult(errors.New("failed to parse addr '"+host.Host+"' ip '"+addrs[0]+"' as IP")))
+ continue
+ }
+
+ hostAddr[host.Host] = addrs[0]
+ addrHosts[addrs[0]] = append(addrHosts[addrs[0]], host.Host)
+ }
+
+ if v4 := remoteAddr.To4(); v4 != nil {
+ remoteAddr = v4
+ }
+
+ // TODO handle IPv6
+
+ window := 256 * 10
+ destPort := host.Port
+ dataOffset := 5 // because we have no options?
+ native := TCPHdrNative{
+ SrcPort: uint16(srcPort),
Review Comment:
## Incorrect conversion between integer types
Incorrect conversion of an integer with architecture-dependent bit size from [strconv.Atoi](1) to a lower bit size type uint16 without an upper bound check.
[Show more details](https://github.com/apache/trafficcontrol/security/code-scanning/228)
--
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 #7096: Add health client parent health
Posted by GitBox <gi...@apache.org>.
rob05c commented on code in PR #7096:
URL: https://github.com/apache/trafficcontrol/pull/7096#discussion_r982915522
##########
tc-health-client/sar/sar.go:
##########
@@ -0,0 +1,348 @@
+// package sar implements a syn-ack-rst health ping.
+// It sends a TCP SYN, waits for an ACK, then immediately sends an RST to kill the connection.
+// The primary purpose of this is as a health check, to verify the remote host is reachable, and able and willing to respond.
+package sar
+
+/*
+ * 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.
+ */
+
+import (
+ "errors"
+ "fmt"
+ "net"
+ "strings"
+ "sync"
+ "time"
+
+ "github.com/apache/trafficcontrol/lib/go-llog"
+)
+
+// SAR calls SARAddr if host is an IP address, or SARHost if host is an FQDN.
+func SAR(log llog.Log, host string, port int, timeout time.Duration) (time.Duration, error) {
+ log = llog.LibInit(log)
+ if ip := net.ParseIP(host); ip != nil {
+ return SARAddr(log, host, port, timeout)
+ }
+ return SARHost(log, host, port, timeout)
+}
+
+// SARAddr sends a syn-ack-reset to the given addr.
+// The addr must be an IP.
+// On success, the round-trip time to receive the Ack is returned, with a nil error.
+// The addr is a string representation of an IPv4 or IPv6 address.
+// TODO add optional local addr param
+func SARAddr(log llog.Log, addr string, port int, timeout time.Duration) (time.Duration, error) {
+ log = llog.LibInit(log)
+ remoteAddr := net.ParseIP(addr)
+ if remoteAddr == nil {
+ return 0, errors.New("failed to parse addr '" + addr + "' as IP")
+ }
+ if v4 := remoteAddr.To4(); v4 != nil {
+ remoteAddr = v4
+ }
+
+ localAddrStr, err := GetLocalAddr()
+ if err != nil {
+ return 0, errors.New("getting local address: " + err.Error())
+ }
+
+ localAddr := net.ParseIP(localAddrStr)
+ if localAddr == nil {
+ return 0, errors.New("failed to parse local addr '" + localAddrStr + "' as IP")
+ }
+ if v4 := localAddr.To4(); v4 != nil {
+ localAddr = v4
+ }
+
+ ephemeralPortHolder, err := GetAndHoldEphemeralPort(localAddrStr)
+ if err != nil {
+ return 0, errors.New("failed to listen on ephemeral port: " + err.Error())
+ }
+ defer ephemeralPortHolder.Close()
+
+ srcPort := ephemeralPortHolder.Port()
+
+ // TODO implement Initial Sequence Number ISN per RFC9293§3.4.1
+ seqNum := uint32(42)
+ window := 256 * 10
+ destPort := port
+ dataOffset := 5 // because we have no options?
+
+ native := TCPHdrNative{
+ SrcPort: uint16(srcPort),
Review Comment:
False positive, bounds are checked in GetAndHoldEphemeralPort.
--
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 #7096: Add health client parent health
Posted by GitBox <gi...@apache.org>.
rob05c commented on code in PR #7096:
URL: https://github.com/apache/trafficcontrol/pull/7096#discussion_r983631749
##########
tc-health-client/sar/multisar.go:
##########
@@ -0,0 +1,388 @@
+// package sar implements a syn-ack-rst health ping.
+// It sends a TCP SYN, waits for an ACK, then immediately sends an RST to kill the connection.
+// The primary purpose of this is as a health check, to verify the remote host is reachable, and able and willing to respond.
+package sar
+
+/*
+ * 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.
+ */
+
+import (
+ "errors"
+ "net"
+ "os"
+ "sync"
+ "time"
+
+ "github.com/apache/trafficcontrol/lib/go-llog"
+)
+
+type HostPort struct {
+ Host string
+ Port int
+}
+
+type SARResult struct {
+ Host string
+ Port int
+ RTT time.Duration
+ Err error
+}
+
+// MultiSAR is like SAR for multiple requests.
+// SAR has to listen on a raw IP port without a TCP socket, which is relatively inexpensive for a single request,
+// but expensive for large numbers of requests.
+// MultiSAR uses a single listener on an ephemeral local port for all SAR requests, significantly reducing
+// resource costs.
+func MultiSAR(log llog.Log, hosts []HostPort, timeout time.Duration) ([]SARResult, error) {
+ log = llog.LibInit(log)
+
+ localAddrStr, err := GetLocalAddr()
+ if err != nil {
+ return nil, errors.New("getting local address: " + err.Error())
+ }
+
+ localAddr := net.ParseIP(localAddrStr)
+ if localAddr == nil {
+ return nil, errors.New("failed to parse local addr '" + localAddrStr + "' as IP")
+ }
+ if v4 := localAddr.To4(); v4 != nil {
+ localAddr = v4
+ }
+
+ ephemeralPortHolder, err := GetAndHoldEphemeralPort(localAddrStr)
+ if err != nil {
+ return nil, errors.New("failed to listen on ephemeral port: " + err.Error())
+ }
+ defer ephemeralPortHolder.Close()
+
+ srcPort := ephemeralPortHolder.Port()
+
+ // pre-construct all the packets, so we listen for as little time as possible
+
+ // TODO implement Initial Sequence Number ISN per RFC9293§3.4.1? It might be faster to use the same seq num for all packets
+ seqNum := uint32(42)
+
+ type HostPortPacket struct {
+ HostPort
+ TCPHdr TCPHdr
+ }
+
+ packets := []HostPortPacket{}
+
+ hostAddr := map[string]string{} // map[host]addr - note hosts may be IPs, in which case host and addr will be the same
+ addrHosts := map[string][]string{} // map[addr][]host - note multiple FQDNs may have the same IP
+
+ results := []SARResult{}
+
+ for _, host := range hosts {
+ makeHostErrResult := func(err error) SARResult {
+ return SARResult{
+ Host: host.Host,
+ Port: host.Port,
+ RTT: 0,
+ Err: err,
+ }
+ }
+
+ remoteAddr := net.ParseIP(host.Host)
+ if remoteAddr != nil {
+ // host is IP
+ hostAddr[host.Host] = host.Host
+ addrHosts[host.Host] = append(addrHosts[host.Host], host.Host)
+ } else {
+ // host isn't an IP, assume FQDN
+ addrs, err := net.LookupHost(host.Host)
+ if err != nil {
+ results = append(results, makeHostErrResult(errors.New("lookup up host '"+host.Host+"': "+err.Error())))
+ continue
+ }
+ if len(addrs) == 0 {
+ results = append(results, makeHostErrResult(errors.New("looking up host '"+host.Host+"' succeeded, but no addresses were found.")))
+ continue
+ }
+ remoteAddr = net.ParseIP(addrs[0])
+ if remoteAddr == nil {
+ results = append(results, makeHostErrResult(errors.New("failed to parse addr '"+host.Host+"' ip '"+addrs[0]+"' as IP")))
+ continue
+ }
+
+ hostAddr[host.Host] = addrs[0]
+ addrHosts[addrs[0]] = append(addrHosts[addrs[0]], host.Host)
+ }
+
+ if v4 := remoteAddr.To4(); v4 != nil {
+ remoteAddr = v4
+ }
+
+ // TODO handle IPv6
+
+ window := 256 * 10
+ destPort := host.Port
+ dataOffset := 5 // because we have no options?
+ native := TCPHdrNative{
+ SrcPort: uint16(srcPort),
Review Comment:
False positive, bounds are checked in GetAndHoldEphemeralPort
--
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