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 2020/04/09 22:28:36 UTC

[GitHub] [trafficcontrol] rob05c opened a new pull request #4628: Add ORT Rewrite Blueprint

rob05c opened a new pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628
 
 
   Adds a ORT Rewrite proposal Blueprint
   
   No tests, it's a blueprint.
   No docs, it's a blueprint.
   No changelog, it's a blueprint.
   
   - [x] This PR fixes is not related to any Issue <!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
   
   
   ## Which Traffic Control components are affected by this PR?
   None, it's a blueprint.
   
   ## What is the best way to verify this PR?
   Read the blueprint, verify you like it.
   
   ## If this is a bug fix, what versions of Traffic Control are affected?
   Not a bug fix, it's a blueprint.
   
   ## The following criteria are ALL met by this PR
   - [x] This PR includes tests OR I have explained why tests are unnecessary
   - [x] This PR includes documentation OR I have explained why documentation is unnecessary
   - [x] This PR includes an update to CHANGELOG.md OR such an update is not necessary
   - [x] This PR includes any and all required license headers
   - [x] This PR ensures that database migration sequence is correct OR this PR does not include a database migration
   - [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
   
   ## Additional Information
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406978228

Review comment:
Agree, with both points.

----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] rob05c commented on issue #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

rob05c commented on issue #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#issuecomment-612160310
 
 
   >On the other hand, if the plan is also to include the necessary parts of ATSTCCFG as part and parcel of the utilities (as opposed to the separate utility it is today) 
   
   The plan is for each app to be small and independent. They could be totally different languages if we want. The config generator is one app, and will probably be a modified version of `atstccfg`. But there's nothing preventing the other apps from being in other languages.
   
   >I'd be happy to help move ORT.py
   
   I'm not opposed to that. Personally, I'd prefer we were more open to more diverse languages. Especially for small apps like these, where it's not a big deal if a new primary developer doesn't like the language, they can just rewrite it, because it's very small.
   
   I also don't object to Python specifically, especially for small things, especially if it includes type annotations and compiling in the build.
   
   If you're the one writing these apps, I wouldn't have any objections. But I'd prefer to leave it up to whoever writes it. Personally, Go would be faster for me and I already have a good grasp of traffic_ops_ort.pl at this point. But I may not be the one writing it, either.
   
   >parts of ATSTCCFG
   
   I'd strongly encourage referring to the command in its proper case, atstccfg. In technical writing it's imperative to not confuse the reader. The application is `atstccfg`, executing `./atstccfg` will not work.
   
   I'm not aware of a universally-accepted technical writing style guide, but the Microsoft Style Guide is one of the most popular. It says 
   https://docs.microsoft.com/en-us/style-guide/capitalization
   
   >Capitalize the first word of labels and terms that appear in UI and APIs unless they're always lowercase (for example, fdisk).
   > In programming languages, follow the traditional capitalization of keywords and other special terms.
   
   And
   
   >Always capitalize the first word of a new sentence. Rewrite sentences that start with a word that's always lowercase.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [trafficcontrol] alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406966422

Review comment:
You say it outputs config files. Does it output them to disk or just stdout to be written by the caller? I ask because you mention multipart/mixed again. And something needs to do things like determining file locations, file permissions, file update strategy (truncate and write vs. new file and copy), backup strategy, and managing parallel processing for performance.

It feels like this component should probably be called like `cat to-data | ort-cfg /etc/trafficserver/records.config` and be expected to manage all except the parallel processing, which the caller would manage by limiting the number of to-data calls.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406969947

##########
File path: blueprints/ort-rewrite-unix-style.md
##########
@@ -0,0 +1,210 @@
+
+# ORT Rewrite in UNIX Philosophy
+
+## Problem Description
+ORT is:
+- Difficult to maintain. Writing Perl is difficult, and reading it is even more difficult.
+- Dangerous to modify. Perl is not compiled, and even validity checks (`perl –c`) fail to verify dynamic runtime errors. This makes it very easy to introduce a bug in seldom-executed areas.
+- Untested. Perl ORT has no unit or integration tests.
+- Opaque. Nobody really knows everything it does, or when, or why.
+
+## Proposed Change
+
+ORT will be rewritten into a series of standalone executables, in the "UNIX Philosophy"
+
+> 1. Make each program do one thing well. To do a new job, build afresh rather than complicate old programs by adding new "features".
+> 2. Expect the output of every program to become the input to another, as yet unknown, program. Don't clutter output with extraneous information. Avoid stringently columnar or binary input formats. Don't insist on interactive input.
+
+- Each executable should do exactly 1 thing, and if a new "thing" becomes necessary, a new executable will be created.
+- The input and output of executables should be text which is easily parseable, so the executables can easily be pipelined (passing the output of one to the input of another), as well as easily read by humans and manipulated by standard Linux/POSIX tools.
+
+This makes ORT:
+- Easier to maintain. Each binary does one thing, is much smaller, and is more obvious. Presumably they’re also written in a language easier to read and write, such as Go.
+- Safer to modify. If each component is smaller, it’s more obvious what it does. We also presume the apps will be written with good development practices (such as modularization), with a language which verifies more at compile-time, and with tests.
+- Clear and easy for operators to understand what each app does. We assume clean interfaces, and good documentation (ideally in the app itself, via help flags, printing usage when no arguments are received, and/or man pages).
+
+#### Implementation
+
+The implementation should adhere to the "UNIX Philosophy," POSIX, Linux Standard Base (LSB), and GNU as much as possible.
+
+ORT will continue to consist of a single OS package (e.g. RPM), which installs all executables.
+
+ORT will require the following executables:
+- **Aggregator**. This is the “primary application” which will emulate the existing ORT script, and be called by CRON or operators to deploy all configs, as ORT does today. Note this is similar to how git works, and several other common Linux CLI utilities.
+ This app will have no logic itself, except to call the other executables.
+ - INPUT: configuration and specification to fetch and emplace config files.
+ - BEHAVIOR: fetches and places config files
+ - OUTPUT: success or failure message
+
+- **Traffic Ops Requestor**. This will fetch data needed from Traffic Ops, such as the Update Pending flag, packages, etc. This should never modify TO data, and should be guaranteed read-only. Any status modifications should go in the Traffic Ops Updater.
+ - INPUT: Traffic Ops URL and credentials, and data to fetch
+ - BEHAVIOR: Requests data from Traffic Ops
+ - OUTPUT: Traffic Ops data requested
+ - Format is probably multipart/mixed, but format may be different if a better format is determined. Ideal "UNIX Philosophy" format is line-delimited text, but the complexity may preclude that. The more complex and difficult to parse, the further from the "UNIX Philosophy." E.g. multipart/mixed is preferable to JSON.
+- **Config File Generator**. This will take TO data and produce config files.
+ - INPUT: Traffic Ops data, and config file(s) to generate or exclude (typically all, possibly “reval only” or other behaviors of ORT)
+ - BEHAVIOR: No side effects. Computationally: builds requested files.
+ - OUTPUT: Config files
+ - Format is probably multipart/mixed, but as above, may be different, multipart is preferable to JSON, etc.
+- **Config File Preprocessor**. Preprocesses generated config files, making post-generation modifications such as underscore directive replacements, and remap OVERRIDE replacements. TODO: determine if this should be rolled into the Config File Generator.
+ - INPUT: Config files, TO data/metadata.
+ - BEHAVIOR: No side effects; computationally: modifies files per rules and data.
+ - OUTPUT: modified config files.
+- **Server Config Readiness Verifier**. Verifies the operating system is ready and safe to apply the given config. Currently, this is just checking udev rules and verifying ATS block devices don’t have filessytems. But it may be more in the future. TODO: determine if necessary; should this even be ORT’s job? Will we ever need anything besides udev/50-ats.rules?
+ - INPUT: config files
+ - BEHAVIOR: No side effects; reads configs and inspects server state.
+ - OUTPUT: whether server is safe and ready to apply config files.
+- **ATS Plugin Readiness Verifier**. Verifies ATS has the necessary plugins for the config files.
+ - INPUT: config files
+ - BEHAVIOR: No side effects; reads configs and inspects server state.
+ - OUTPUT: whether ATS has all necessary plugins, or which files require which missing plugins.
+- **Diff Tool**. This will take two config files (presumably an existing and new file) and return their differences. TODO: determine if this is necessary; if configs are deterministic, can POSIX diff be used?
+ - INPUT: Two config files
+ - BEHAVIOR: No side effects. Computationally: diffs given files.
+ - OUTPUT: file diff. Ideally in a standard format.
+- **Backup Tool**. This will take a file and copy it to a backup location. TODO: determine if necessary; are standard POSIX cp/mv/etc enough?
+ - INPUT: config file to backup, and backup location. May be text of new file or path of existing file.
+ - BEHAVIOR: Save or copy given file to given backup location. Should be atomic.
+ - OUTPUT: success or failure message.
+- **Restart Determiner**. Takes the config to be applied (only the changed files, after diffing), encapsulates the logic of what changes require a reload or restart, and returns whether a restart, reload, or neither is required.
+ - INPUT: config files to be applied.
+ - BEHAVIOR: no side effects; computationally: inspects files and determines action.
+ - OUTPUT: whether ATS needs reloaded or restarted.
+- **Service Reloader**. Takes the name of the service (possibly only ATS) to reload or restart, and reloads or restarts as necessary. TODO determine if necessary; is this any logic beyond calling service restart and/or traffic_ctl?
+ - INPUT: service, whether to reload or restart
+ - BEHAVIOR: reloads or restarts the service
+ - OUTPUT: success or failure
+- **Traffic Ops Updater**. This will set the server’s update status in Traffic Ops. This should be only the Update and Reval Pending flags; ORT should never modify server configuration data, only ever server configuration status data.
+ - INPUT: Traffic Ops URL and credentials, and status to set
+ - BEHAVIOR: Makes a POST request to TO setting the status
+ - OUTPUT: success or failure message
+
+#### Features Omitted
+
+The following features of the current ORT are specifically not being implemented in the redesign:
+
+- **Chkconfig**. Chkconfig is not used by CentOS 7+; specifically, SystemD does not use it. It is misleading that ORT sets it today.
+- **Ntpd**. ORT currently has custom logic to restart ntpd if an ntpd.conf is changed. This should be managed by whatever system is managing the server (Ansible/Puppet/Manual/etc). Network time should not be the responsibility of Traffic Control or its config applicator.
+- **Interactive mode**. This mode is rarely possibly never used today. Further, by dividing ORT into UNIX-style apps for each function, an operator can easily see what results would be before running them.
+- **Revalidate Mode**. ORT is now fast enough to make a separate Revalidate unnecessary. It should always check and apply all files.
+- **Report Mode**. The Aggregator itself does not need a Report Mode. Operators wanting to see what would be changed can run the Traffic Ops Requestor, Config File Generator, Diff Tool, Restart Determiner, and Package Install Determiner.
+- **Package Installation**. ORT will cease to perform this. OS (RPM) package installation will no longer be done by Traffic Control, but rather by whoever or whatever is managing the machine and operating system (Ansible, Puppet, human system administrators, etc).
+ - Whatever is managing the other hundreds of packages on the operating system should also manage ATS and its plugins. ORT's job is to manage Traffic Control configuration data, not the operating system.
+
+#### Additional Utilities.
+
+Shell scripts which are “one-liners” pipelining common operations should be provided with the OS Package. TODO: add a list of scripts under Implementation heading.
+
+Additionally, a .pl script which emulates the existing ORT behavior will be provided in the old location, to preserve backwards-compatibility. This script should be very small, and essentially translate old calls and flags to the new Aggregator.
+
+#### Notes
+
+I started to list requirements for all apps, such as unit tests, integration tests, modular design, argument/manpage for usage info, etc. But then I realized I was just listing good design principles. So I decided to omit that.
+
+### Traffic Portal Impact
+None.
+
+### Traffic Ops Impact
+None.
+
+#### REST API Impact
+None.
+
+#### Client Impact
+None.
+
+#### Data Model / Database Impact
+None.
+
+### ORT Impact
+Completely rewrites ORT. Backward-compatibility for safe upgrades will be preserved.
+
+The Interface will be similar, but compatibility is not a goal. Further, LSB-compliant options and parameters is a goal, and will require incompatibility.
+
+A `traffic_ops_ort.pl` script will be provided, whose interface _does_ preserve backward compatibility, and calls the new ORT "aggregator". This will preserve existing CRON or other tools users are using with ORT, and make an upgrade not break a production system.
+
+### Traffic Monitor Impact
+None.
+
+### Traffic Router Impact
+None.
+
+### Traffic Stats Impact
+None.
+
+### Traffic Vault Impact
+None.
+
+### Documentation Impact
+Rewrite will provide MAN pages for ORT. ORT arguments are not currently documented, and that will not be changed, to avoid duplicate documentation. TC "read-the-docs" may include a small comment pointing users to the MAN page.
+
+### Testing Impact
+
+ORT does not currently include an Integration Test framework. Creating one is orthogonal to this project, but this project is a good opportunity to do so at the same time.
+
+ORT Integration Tests need exactly the same thing as the Traffic Ops API tests: a running TO instance populated with data. It is suggested that an ORT Integration Test framework copy or abstract the TO API Tests.
+
+### Performance Impact
+Not significant. ORT in master as of this writing takes less than a minute to run, on a large CDN with at least 1000 Delivery Services and 1000 Servers. The Rewrite should still take less than a minute to run.
+
+It should be a goal for ORT to take less than 15 seconds to run, given a fast Traffic Ops. Because a faster ORT means TC changes propogate faster, which is ideal. But ORT is not in the Request Path, so performance is not critical to TC operation. Further, the previous release of ORT took 5-8 minutes on a large CDN.
+
+### Security Impact
+None.
+
+### Upgrade Impact
+None. The rewrite will provide a compatibility script, so existing tools and runs continue to work as before.
+
+### Operations Impact
+Operators should learn the new real ORT interface, to better operate Traffic Control. However, it's not strictly required, they can continue to use the compatibility script for the immediate future.

Review comment:
We should be clear about when they'll need to upgrade to the new systems. It's important that operators have a timeline for their transition.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406976355

Review comment:
The config generator outputs to stdout. The Aggregator that's acting as a single "ORT run" will then write them to disk (after comparing, diffing, etc).

We could have another tool for that, to take config files as input and write them to files. So your example would be `cat to-data | ort-cfg | ort-write-files`. That didn't seem necessary to me, but I'm not strongly opposed.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] jrushford commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

jrushford commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r407569182

Review comment:
I was thinking that the "Server Config Readiness Verifier" shouldn't do too much beyond verifying as Chris mentioned that the server was not ONLINE and leave checking UDEV rules and OS things to other tools such as Ansible but the "Server Config Readiness Verifier" should definitely insure that the proper ATS RPM's are installed if something else is installing them.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] jrushford commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

jrushford commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r407569182

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406978865

Review comment:
+1
In fact, the ideal would be if the same file is both the README.md in the app's directory for Github, and used to generate the man. But IMO man and not duplicating text should take priority.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406967366

Review comment:
I assume this uses the return code to signal failure?

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406978038

Review comment:
>Maybe a backup file name

I assume the backup filename is input to this app, and the Aggregator can pick that. The Aggregator could even read a TO Parameter (the answer to all problems) if we wanted. Though that feature doesn't exist today.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406967759

Review comment:
Even if configs are deterministic, a diff output for humans will be very valuable. And POSIX diff on things like YAML is basically useless.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] rob05c edited a comment on issue #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

rob05c edited a comment on issue #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#issuecomment-612160310
 
 
   >On the other hand, if the plan is also to include the necessary parts of ATSTCCFG as part and parcel of the utilities (as opposed to the separate utility it is today) 
   
   The plan is for each app to be small and independent. They could be totally different languages if we want. The config generator is one app, and will probably be a modified version of `atstccfg`. But there's nothing preventing the other apps from being in other languages.
   
   >I'd be happy to help move ORT.py
   
   I'm not opposed to that. Personally, I'd prefer we were more open to more diverse languages. Especially for small apps like these, where it's not a big deal if a new primary developer doesn't like the language, they can just rewrite it, because it's very small.
   
   I also don't object to Python specifically, especially for small things, especially if it includes type annotations and compiling in the build.
   
   If you're the one writing these apps, I wouldn't have any objections. But I'd prefer to leave it up to whoever writes it. Personally, Go would be faster for me and I already have a good grasp of traffic_ops_ort.pl at this point. But I may not be the one writing it, either.
   
   >parts of ATSTCCFG
   
   I'd strongly encourage referring to the command in its proper case, atstccfg. In technical writing it's imperative to not confuse the reader. The application is `atstccfg`, executing `./ATSTCCFG` will not work.
   
   I'm not aware of a universally-accepted technical writing style guide, but the Microsoft Style Guide is one of the most popular. It says 
   https://docs.microsoft.com/en-us/style-guide/capitalization
   
   >Capitalize the first word of labels and terms that appear in UI and APIs unless they're always lowercase (for example, fdisk).
   > In programming languages, follow the traditional capitalization of keywords and other special terms.
   
   And
   
   >Always capitalize the first word of a new sentence. Rewrite sentences that start with a word that's always lowercase.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [trafficcontrol] alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406968326

Review comment:
This tool could be clever about the operating system it's working on. Not sure it's totally necessary for a first release, though.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406967186

Review comment:
Could this also do things like checking to be sure that the server is not currently ONLINE or is otherwise fit for service? Might it verify that the OS matches what TO expects to deploy into? I think this might do other things, but some of them might need a read-only TO credential.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406979220

Review comment:
Fixed

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406968033

Review comment:
Feels like standard tools are probably the right call here. Maybe a backup file name generator, though? It really kinda depends what the backup strategy is.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] ocket8888 commented on issue #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

ocket8888 commented on issue #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#issuecomment-611797488
 
 
   Your blueprint doesn't mention a decided language - though offers Go as a suitable candidate.
   For what it's worth, I'd be happy to help move ORT.py in this direction. It has the advantage of having transliterated most of ORT into separate modules - which could make dividing it into separate utilities simpler than starting from scratch.
   
   On the other hand, if the plan is also to include the necessary parts of ATSTCCFG as part and parcel of the utilities (as opposed to the separate utility it is today) then Go is probably a better choice. Because the configuration file generation is the hardest part to get right.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

ocket8888 commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r407687987

Review comment:
Sphinx has an output mode for manual pages. So the official docs for the utility could also be its man pages.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406972111

Review comment:
The Blueprint doesn't go in that level of detail. I'm +1 on both of those.

I think it's implied "all behavior not specifically stated as being removed will be preserved"

I see the Blueprint as the larger design idea, for community approval. Small features like that, I'd vote we make as regular Github Issues.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406977143

Review comment:
Yes, and also empty output probably means success, and non-empty output probably means an error. I trust the implementer to do the thing is most in the Unix Philosophy. And to document it in the man page.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406966704

Review comment:
IMO, roll this into the generator. And maybe remove as much that depends on it as possible. :)

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406969085

Review comment:
This is a critically important stepping stone in the process, but it may prove to be a pain to maintain in the future. Let's make sure we're clear that this tool is deprecated via a formal deprecation notice with plenty of fanfare so folks are aware. We don't want this shim to live forever.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406970110

Review comment:
s/significnatlly/significantly/

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406961909

Review comment:
There are other important details about fetching data, like what sorts of timeouts, proxies, or caches should be used. Should these be inputs to the requestor or generic configuration you find elsewhere. Similarly, should credentials be passed each time or should they be available in a protected file the caller can access?

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406976811

Review comment:
It could. I'm not opposed, but I'd vote we put details like that in Github Issues, and keep this design proposal higher-level.

>I think this might do other things, but some of them might need a read-only TO credential.

If so, the TO Data Fetcher should get that data.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406970486

Review comment:
Yeah, it will definitely have compile-time dependencies unless you plan to write an HTTP client in whatever language you use. At a minimum, it will depend on the standard library. All that should be fine, though. It's hard to guess which dependencies you'll need from the get-go.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406968618

Review comment:
I mentioned this on the list, so I won't go in detail here, but I think this should be considered for inclusion.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406975554

Review comment:
IMO JSON on the CLI strongly violates the UNIX Philosophy. Unix apps should return columnar text, for ease of parsing, and to interoperate with other tools.

multipart is very not-ideal either; but we need a way to delimit files. You can almost do two newlines, but you at least need the metadata of the file name, and other metadata is really useful too. Which, if you were making a custom format, leads you to something very similar to multipart.

JSON is a tree. It's nice that it's a simple language, but trees are drastically more complex than lines.

I wish there were something closer to the standard UNIX line-delimiting, but I don't think there is. Any tree format is way more complex to parse, search, hold in memory, etc. It basically makes operating on with the standard UNIX toolset (awk/grep/sed/etc) impossible, without `jq`. Where multipart is at least possible to use the standard tools on, if slightly more complex from the two delimiter levels.

You're right, JSON is more popular than multipart. But multipart is really just a delimiter and headers, it's super-easy to parse. And most languages do have libraries for it.

It took me maybe an hour to do it in Perl, language I'm very much not an expert in:
https://github.com/apache/trafficcontrol/blob/5693451bb898f62d0e14afa60a9af9824d3c8e06/traffic_ops/ort/traffic_ops_ort.pl#L1600
And e.g. Go has libraries for it: https://golang.org/pkg/mime/multipart/

I could not write a JSON parser in an hour. And no, I won't need to. But, it shouldn't even be a possibility. _Whatever_ has to be parsed should be as easy, human readable, and line-delimited as possible.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

rob05c commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406977584

Review comment:
Not opposed to a smarter diff. But:

>POSIX diff on things like YAML is basically useless.

Why? YAML is line-based. And even if a config weren't, the operator can and should format their code on lines, so the diff does make sense. I can't imagine a config needing to be minified

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406969732

Review comment:
Consider having man pages generated from Markdown so that they can easily be read from both Github directly, the file system of a developer working on them, and via man after install.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [trafficcontrol] alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint

Posted by GitBox <gi...@apache.org>.

alficles commented on a change in pull request #4628: Add ORT Rewrite Blueprint
URL: https://github.com/apache/trafficcontrol/pull/4628#discussion_r406964111

Review comment:
I disagree. JSON or similar should generally be preferable to multipart/mixed because there are better and more plentiful tools for transforming it. And if the objects returned may be large, perhaps application/json-seq is a good job. (See [RFC 7464](https://tools.ietf.org/html/rfc7464).) Or even [JSONLines](http://jsonlines.org/).

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services