Skip to content

New Instructions for installing snap including enabling interfaces#37

Closed
sklassen wants to merge 19 commits intoapache:masterfrom
sklassen:snap_instructions
Closed

New Instructions for installing snap including enabling interfaces#37
sklassen wants to merge 19 commits intoapache:masterfrom
sklassen:snap_instructions

Conversation

@sklassen
Copy link
Copy Markdown
Contributor

@sklassen sklassen commented Nov 5, 2018

Overview

I've reorganized the READ.md for the snap installation. I added an important note about permissions for interfaces. Build instructions have been moved to BUILD.md

Testing recommendations

No change to configuations

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;

sklassen and others added 4 commits November 6, 2018 00:11
Spelling and formatting improvements
Correct spelling mistake
More spelling errors
@wohali
Copy link
Copy Markdown
Member

wohali commented Nov 5, 2018

@sklassen please add the snap permissions steps to your HOWTO.md file. Once that is done I can merge this.

FYI I will be publishing the 2.2.0 snap as an edge release only, does that require changing the snap installation command you provide under README.md? If so, please update that as well.

@wohali
Copy link
Copy Markdown
Member

wohali commented Nov 5, 2018

One more change:

In this line: https://github.com/apache/couchdb-pkg/blob/master/snap/snap_run#L19

you change the order of ini file handling so that setting snap configurations persists correctly. You need to tell end users that you've changed CouchDB's config processing behaviour, otherwise the snap will not behave the same as we document here: http://docs.couchdb.org/en/stable/config/intro.html#configuration-files

Please include a warning about this in /README.md.

@wohali
Copy link
Copy Markdown
Member

wohali commented Nov 5, 2018

@sklassen actually, I see more issues now. I should have read your other PR more carefully.

Copy link
Copy Markdown
Member

@wohali wohali left a comment

Choose a reason for hiding this comment

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

Sorry, I rushed through your other PR. I think this one key rework is required.

I realize that you may already have CouchDB machines deployed with your other approach, and I'm sorry for that inconvenience. But if we can validate the approach I've suggested here, it'll be better for everyone in the long run. Matching behaviour of the other official releases is key.

Comment thread snap/HOWTO.md Outdated
@sklassen
Copy link
Copy Markdown
Contributor Author

sklassen commented Nov 5, 2018

@sklassen please add the snap permissions steps to your HOWTO.md file. Once that is done I can merge this.

This should be there already (line 14 in the new file) in correct order.

FYI I will be publishing the 2.2.0 snap as an edge release only, does that require changing the snap installation command you provide under README.md? If so, please update that as well.

Sure, added to README and HOWTO.

One more change:

In this line: https://github.com/apache/couchdb-pkg/blob/master/snap/snap_run#L19

you change the order of ini file handling so that setting snap configurations persists correctly. You need to tell end users that you've changed CouchDB's config processing behaviour, otherwise the snap will not behave the same as we document here: http://docs.couchdb.org/en/stable/config/intro.html#configuration-files

Please include a warning about this in /README.md.

I had a description of the configuration order, and I thought that the more familiar HTTP should supersede the new and less known snap set. But I have no fixed opinion. I can reverse it back. /etc/local.ini and then /etc/local.d/ and make that clear in the README.md

I can move the couchdb.ini with the data directory into default.ini

Any changes to couchdb from the http configutation tool are made here

/etc/local.d/local.ini was a typo: it should, of course, have been /etc/local.ini

hooks/configure is a mess. You're creating a huge number of .ini files here. When any changes are made via the http interface (via /_node/_local/_config) these changes will end up in the last file in the config chain.

Which may have been my idea of leave local.ini last in the list, if that is the file chosen by the http config writer. I can added sequence numbers if that turns out to be the case.

My original idea was to code hook/configure so that it only changed vm.args and those items that had not been whitelisted for http configure, plus a few useful items. But the list of blacklists was long; while what I thought as useful kept growing. And I do need one file per section otherwise the bash script becomes too complicated (I tried to limit it to sed and not awk). But I'm happy to cut it back to the bare essentials and give some more examples of using HTTP configure for the other. HTTP configure does have the advantage of not requiring a restart. Do you have an opinion on which items to keep or numbers to order the configuration?

Debian/Ubuntu/CentOS/RedHat packages, we use default.d/10-filelog.ini to log to a file, not to stdout by default,
That might make sense when dealing with so many distributions. But here I think the snap should default to stdout and picked up by journald. snap startup and config changes are already closely tied into the journald and if you are using ubuntu to host a cluster of couchdb, then the centralized monitoring via journactl across hosts is useful. It also writes in binary for added performance. I also wanted to avoid writting to /var/snap/couchdb/common/log as this will overwrite between instances; and also avoid writting in each /var/snap/couchdb/xx/log because then large log files after upgrade may never be rotated. journalctl handles both problems nicely.

There's quite a few changes. I've started, but leave some for tomorrow and test against a fresh installation. I'll revert soon.

@sklassen
Copy link
Copy Markdown
Contributor Author

sklassen commented Nov 6, 2018

Okay, all good suggestions, I have done, I believe, as requested.

  1. ERL_FLAGS is still there (I do need it to split /snap and /var/snap) but the order is now standard
  2. couchdb.ini is now in /etc/default.d
  3. configure hook now writes out sequence 10,20,30 ini files in /etc/local.d; far fewer options now.
  4. installation hook writes a blank /etc/local.ini and /etc/local.d/90-override.ini for the configuration over http.

I tested with an upload the edge channel of couchdb-sklassen

Anything missed?

@wohali
Copy link
Copy Markdown
Member

wohali commented Nov 7, 2018

@sklassen It'd be best if you could leave the etc/local.ini file that we ship rather than writing a blank one. That way, the sample text that we provide is available if people choose to edit that file by hand rather than use some other way to update it. As everything in our copy is commented out, there is no impact to how your snap will operate.

Copy link
Copy Markdown
Member

@wohali wohali left a comment

Choose a reason for hiding this comment

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

One more change.

Comment thread snap/HOWTO.md Outdated
@wohali
Copy link
Copy Markdown
Member

wohali commented Nov 7, 2018

Finally, you'll want to be aware of:

apache/couchdb#1602

which will change how the query servers are activated. This will land before 2.3.0 is released. That means your code will need changing again.

@sklassen
Copy link
Copy Markdown
Contributor Author

sklassen commented Nov 7, 2018

@sklassen It'd be best if you could leave the etc/local.ini file that we ship rather than writing a blank one. That way, the sample text that we provide is available if people choose to edit that file by hand rather than use some other way to update it. As everything in our copy is commented out, there is no impact to how your snap will operate.

Okay, I've done that in the install hook. Fresh installations will have it copied over; Updates will copy from the previous revision's /etc/local.ini.

Finally, you'll want to be aware of:

apache/couchdb#1602

which will change how the query servers are activated. This will land before 2.3.0 is released. That means your code will need changing again.

Okay, noted. I'm a fan of the native erlang views. When I test the 2.3.0 snap, I will make the code change.

@sklassen sklassen closed this Nov 7, 2018
@wohali
Copy link
Copy Markdown
Member

wohali commented Nov 7, 2018

@sklassen Why did you close the PR? I haven't merged it yet. Are you going to open a new one?

Be careful you don't clobber the user's own local.ini file. If they've made their own changes, you don't want to overwrite it with the updated version.

You should check out mango indexes, in my recent testing they were even faster than erlang views.

@sklassen
Copy link
Copy Markdown
Contributor Author

sklassen commented Nov 7, 2018 via email

@sklassen
Copy link
Copy Markdown
Contributor Author

sklassen commented Nov 7, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants